Skip to content

Conversation

kzvieriev
Copy link
Contributor

Functional tests for VolatileLayerClient::PrefetchTiles() method. New
catalog/credentials created for the tests, since Read/Write volatile
layer with HERETile scheme required.

Resolves: OLPEDGE-1738

Signed-off-by: Kostiantyn Zvieriev ext-kostiantyn.zvieriev@here.com

@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #772 into master will increase coverage by 0.4%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #772     +/-   ##
========================================
+ Coverage    78.2%   78.7%   +0.4%     
========================================
  Files         289     292      +3     
  Lines        9709    9931    +222     
========================================
+ Hits         7597    7811    +214     
- Misses       2112    2120      +8     
Impacted Files Coverage Δ
...nclude/olp/dataservice/read/PrefetchTilesRequest.h 68.0% <0.0%> (-24.0%) ⬇️
...aservice/write/model/PublishPartitionDataRequest.h 84.2% <0.0%> (-15.8%) ⬇️
...p-cpp-sdk-core/src/cache/DiskCacheSizeLimitEnv.cpp 83.3% <0.0%> (-3.3%) ⬇️
olp-cpp-sdk-core/src/utils/Dir.cpp 78.3% <0.0%> (-2.4%) ⬇️
olp-cpp-sdk-core/src/cache/DiskCache.cpp 65.8% <0.0%> (-2.1%) ⬇️
olp-cpp-sdk-core/src/utils/Url.cpp 57.7% <0.0%> (-0.5%) ⬇️
olp-cpp-sdk-core/src/utils/Base64.cpp 19.1% <0.0%> (ø)
olp-cpp-sdk-core/src/cache/DefaultCacheImpl.h 100.0% <0.0%> (ø)
olp-cpp-sdk-core/include/olp/core/http/Network.h 100.0% <0.0%> (ø)
...-cpp-sdk-core/include/olp/core/http/NetworkTypes.h 100.0% <0.0%> (ø)
... and 11 more

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 8c4e055...9abc7ba. Read the comment docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be done in fixture SetUp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prefetch tests need different setup. They use another catalog, credentials etc. + we need to write tiles data used only by these tests, so moving it to SetUp() will lead to overhead for other test.

Copy link
Contributor

Choose a reason for hiding this comment

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

could we have one app id and secret for all tests in VolatileLayerClientTest? So other user can set only once their id/secret.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will need to change old tests, so they use new catalog, layers etc.
Old credentials/catalog can't be used in new tests, since need to write data with tiles first.

Anyway, there is a separated task to create accessible catalogs for all tests. After that we could use same credentials for all tests here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also could we use SetUp function for prefetch tests also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reason here, prefetch tests require tiles in the layer.
Adding data isn't fast process, so I prefer to make this preparation only for these new tests. What do you think about it?

Comment on lines 385 to 396
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need such a functional test. This can be, resp. should be, an integration test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeap. integration tests has same test. Removed.

Comment on lines 398 to 375
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, should be an integration test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed also.

Copy link
Contributor

Choose a reason for hiding this comment

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

These are all integration tests.
The only test you need to do extra is that in case min/max are equal or 0, that only the requested tilekeys are downloaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.
Extra tests for 0/0 and same levels added.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are testing too much. You don't need to replicate the integration tests here. You only need to test the real functionality with OLP, resp. with the mocked server later on. So that you have covered the e2e scenario.
You don't need to replicate the entire integration tests here. This is not the point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.
Actually, I took them from VersionedLayerClient prefetch fv tests. So they need cleanup too.

@kzvieriev kzvieriev requested a review from andescu April 16, 2020 11:46
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't set any TaskScheduler. Please set TaskScheduler else all requests are sync, which we don't want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export dataservice_read_volatile_test_prefetch_catalog=hrn:here:data::olp-here-test:test-volatile-prefetch"
export dataservice_read_volatile_test_prefetch_catalog="hrn:here:data::olp-here-test:test-volatile-prefetch"

andescu
andescu previously approved these changes Apr 23, 2020
Functional tests for VolatileLayerClient::PrefetchTiles() method. New
catalog/credentials created for the tests, since Read/Write volatile
layer with HERETile scheme required.

Resolves: OLPEDGE-1738

Signed-off-by: Kostiantyn Zvieriev <ext-kostiantyn.zvieriev@here.com>
@kzvieriev kzvieriev dismissed stale reviews from LiubovDidkivska and andescu via 9abc7ba April 29, 2020 08:36
@kzvieriev kzvieriev merged commit d5630ed into master Apr 29, 2020
@kzvieriev kzvieriev deleted the task/olpedge-1738 branch April 29, 2020 09:35
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.

5 participants