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

Append nanoseconds to dataset name in test_historical_retrival to prevent tests stomping over each other #1593

Merged
merged 6 commits into from
May 28, 2021

Conversation

achals
Copy link
Member

@achals achals commented May 28, 2021

Signed-off-by: Achal Shah [email protected]

What this PR does / why we need it:
Becuase of the way we construct same dataset name in test_historical_retrival, when running a matrix test there's the possiblity of two different tests constructing the same dataset name, and hence stomping over each other.

This PR adds the python version to the name of the dataset - this should be sufficient since the test matrix tests different python versions. An alternative would be to use the github action run id, but that ties the test a little bit closer to github than seems necessary.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

…ival to prevent tests stomping over each other

Signed-off-by: Achal Shah <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented May 28, 2021

Codecov Report

Merging #1593 (29df7b0) into master (e451a11) will decrease coverage by 6.22%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1593      +/-   ##
==========================================
- Coverage   83.60%   77.38%   -6.23%     
==========================================
  Files          65       64       -1     
  Lines        5758     5633     -125     
==========================================
- Hits         4814     4359     -455     
- Misses        944     1274     +330     
Flag Coverage Δ
integrationtests ?
unittests 77.38% <50.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sdk/python/tests/test_historical_retrieval.py 61.71% <50.00%> (-37.14%) ⬇️
sdk/python/tests/fixtures/data_source_fixtures.py 38.88% <0.00%> (-61.12%) ⬇️
sdk/python/tests/test_cli_gcp.py 40.74% <0.00%> (-59.26%) ⬇️
sdk/python/feast/infra/offline_stores/bigquery.py 32.41% <0.00%> (-58.63%) ⬇️
sdk/python/tests/test_feature_store.py 60.52% <0.00%> (-39.48%) ⬇️
...hon/tests/test_offline_online_store_consistency.py 74.72% <0.00%> (-25.28%) ⬇️
sdk/python/feast/registry.py 59.55% <0.00%> (-19.56%) ⬇️
sdk/python/feast/data_source.py 67.94% <0.00%> (-10.90%) ⬇️
sdk/python/feast/feature_view.py 81.63% <0.00%> (-6.13%) ⬇️
... and 6 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 e451a11...29df7b0. Read the comment docs.

Signed-off-by: Achal Shah <[email protected]>
@achals achals requested a review from woop May 28, 2021 17:59
@woop woop changed the title Append the python version to the dataset name in test_historical_retrival to prevent tests stomping over each other Append nanoseconds to dataset name in test_historical_retrival to prevent tests stomping over each other May 28, 2021
@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, woop

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@woop
Copy link
Member

woop commented May 28, 2021

/lgtm

@feast-ci-bot feast-ci-bot merged commit f543871 into feast-dev:master May 28, 2021
@achals achals deleted the flaky-integration-tests branch May 28, 2021 20:20
woop pushed a commit that referenced this pull request Jun 7, 2021
…vent tests stomping over each other (#1593)

* Append the python version to the dataset name in test_historical_retrival to prevent tests stomping over each other

Signed-off-by: Achal Shah <[email protected]>

* make format

Signed-off-by: Achal Shah <[email protected]>

* use version_info to avoid mucking wiht strings

Signed-off-by: Achal Shah <[email protected]>

* time_ns followed by randint

Signed-off-by: Achal Shah <[email protected]>

* fix lint

Signed-off-by: Achal Shah <[email protected]>

* Use 1000-9999 for fixed width

Signed-off-by: Achal Shah <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants