Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add asynchronous methods for GetLatestVersion functionality #1306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dudnyk
Copy link
Contributor

@dudnyk dudnyk commented Jan 25, 2022

Creation of asynchronous methods is a part of task
continuation changes, later will be used in asynchronous
GetVersion method. GetVersion is not added in this commit
as it's in the private section and cannot be covered with tests.

Add asynchronous version of GetLatestVersion method
which handles the logic of getting data from cache/network.
Add asynchronous version of GetLatestCatalogVersion.
Move common functionality of getting data from the cache
and storing data returned from a request to the cache
to the RetrieveLatestVersion function.

Add unit tests which cover asynchronous methods for getting
the latest version.
Replaced mock expectation arguments with WithoutArgs.

Relates-To: OLPEDGE-2718

Signed-off-by: Yevhenii Dudnyk ext-yevhenii.dudnyk@here.com

@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #1306 (529fa85) into master (b1cd55b) will increase coverage by 0.1%.
The diff coverage is 98.8%.

❗ Current head 529fa85 differs from pull request most recent head 20d7ec1. Consider uploading reports for the commit 20d7ec1 to get more accurate results

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1306     +/-   ##
========================================
+ Coverage    86.1%   86.2%   +0.1%     
========================================
  Files         381     381             
  Lines       13425   13497     +72     
========================================
+ Hits        11565   11636     +71     
- Misses       1860    1861      +1     
Impacted Files Coverage Δ
...aservice-read/src/repositories/CatalogRepository.h 100.0% <ø> (ø)
...ervice-read/src/repositories/CatalogRepository.cpp 99.3% <98.5%> (-0.7%) ⬇️
...dataservice-read/src/generated/api/MetadataApi.cpp 72.4% <100.0%> (+5.0%) ⬆️
...ice-read/src/repositories/CatalogCacheRepository.h 100.0% <0.0%> (ø)
...clude/olp/dataservice/read/CatalogVersionRequest.h 95.8% <0.0%> (+0.6%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1cd55b...20d7ec1. Read the comment docs.

@dudnyk dudnyk changed the title Task/olpedge 2718 Add GetVersionOnline method Jan 26, 2022
@dudnyk dudnyk force-pushed the task/olpedge-2718 branch 15 times, most recently from afd16fe to 21d435a Compare February 1, 2022 09:49
@dudnyk dudnyk changed the title Add GetVersionOnline method Add asynchronous methods for GetLatestVersion functionality Feb 1, 2022
@dudnyk dudnyk force-pushed the task/olpedge-2718 branch 2 times, most recently from 0c57b8f to e93ebbc Compare February 1, 2022 09:54
@dudnyk dudnyk force-pushed the task/olpedge-2718 branch 3 times, most recently from 27bc9ff to aba52e9 Compare February 3, 2022 08:48
@dudnyk dudnyk force-pushed the task/olpedge-2718 branch 2 times, most recently from 9b268e8 to 1ea88ec Compare February 4, 2022 16:15
Creation of asynchronous methods is a part of task
continuation changes, later will be used in asynchronous
GetVersion method. GetVersion is not added in this commit
as it's in the private section and cannot be covered with tests.

Add asynchronous version of GetLatestVersion method
which handles the logic of getting data from cache/network.
Add asynchronous version of GetLatestCatalogVersion.
Move common functionality of getting data from the cache
and storing data returned from a request to the cache
to the RetrieveLatestVersion function.

Add unit tests which cover asynchronous methods for getting
the latest version.
Replaced mock expectation arguments with WithoutArgs.

Relates-To: OLPEDGE-2718

Signed-off-by: Yevhenii Dudnyk <ext-yevhenii.dudnyk@here.com>
static client::CancellationToken GetLatestCatalogVersion(
const client::OlpClient& client, int64_t start_version,
boost::optional<std::string> billing_tag,
read::CatalogVersionCallback callback);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
read::CatalogVersionCallback callback);
CatalogVersionCallback callback);

}
}

return std::move(version_response);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pessimizing-move
More info: https://stackoverflow.com/questions/62061433/how-to-avoid-the-pessimizing-move-warning-of-nrvo

Suggested change
return std::move(version_response);
return version_response;

version_response = std::move(*cached_version);
}

return std::move(version_response);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pessimizing-move
More info: https://stackoverflow.com/questions/62061433/how-to-avoid-the-pessimizing-move-warning-of-nrvo

Suggested change
return std::move(version_response);
return version_response;

@@ -91,8 +98,7 @@ class CatalogRepositoryTest : public ::testing::Test {
settings_.network_request_handler = network_;
settings_.cache = cache_;

lookup_client_ =
std::make_shared<olp::client::ApiLookupClient>(kHrn, settings_);
lookup_client_ = std::make_shared<client::ApiLookupClient>(kHrn, settings_);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a distinct using for this one.

Suggested change
lookup_client_ = std::make_shared<client::ApiLookupClient>(kHrn, settings_);
lookup_client_ = std::make_shared<ApiLookupClient>(kHrn, settings_);

olp::client::OlpClientSettings settings_;
std::shared_ptr<olp::client::ApiLookupClient> lookup_client_;
client::OlpClientSettings settings_;
std::shared_ptr<client::ApiLookupClient> lookup_client_;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::shared_ptr<client::ApiLookupClient> lookup_client_;
std::shared_ptr<ApiLookupClient> lookup_client_;

olp::client::CancellationContext context;
TEST_F(CatalogRepositoryTest, AsyncGetLatestVersionOnlineOnlyForbidden) {
const auto request =
read::CatalogVersionRequest().WithFetchOption(read::OnlineIfNotFound);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have OnlineIfNotFound request, but the name of the test contains OnlineOnly. Also, line 316 contains a mention of OnlineOnly.

}

TEST_F(CatalogRepositoryTest, AsyncGetLatestVersionOnlineOnlyUserCancelled2) {
const auto request = read::CatalogVersionRequest();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A default fetch option is OnlineIfNotFound. But the test name contains OnlineOnly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants