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

Teardown infrastructure after integration tests #1697

Merged
merged 5 commits into from
Jul 13, 2021

Conversation

achals
Copy link
Member

@achals achals commented Jul 8, 2021

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

What this PR does / why we need it:

The test_offline_online_store_consistency.py integration test didn't tear down any infrastructure after running. This led to an issue where we ran past the number of dynamo tables allowed for an account: https://github.com/feast-dev/feast/runs/3014392319?check_suite_focus=true

This PR changes the integration test to tear down the infrastructure set up by the fixtures.

Which issue(s) this PR fixes:

Fixes #1696.

Does this PR introduce a user-facing change?:

NONE

@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2021

Codecov Report

Merging #1697 (f88e12e) into master (0d2179d) will increase coverage by 0.50%.
The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1697      +/-   ##
==========================================
+ Coverage   82.75%   83.25%   +0.50%     
==========================================
  Files          76       76              
  Lines        6754     6778      +24     
==========================================
+ Hits         5589     5643      +54     
+ Misses       1165     1135      -30     
Flag Coverage Δ
integrationtests 83.18% <95.83%> (+0.50%) ⬆️
unittests 69.91% <66.66%> (+0.18%) ⬆️

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

Impacted Files Coverage Δ
sdk/python/feast/feature_store.py 94.35% <92.85%> (-0.09%) ⬇️
...hon/tests/test_offline_online_store_consistency.py 100.00% <100.00%> (ø)
sdk/python/feast/infra/offline_stores/bigquery.py 76.14% <0.00%> (ø)
sdk/python/feast/infra/online_stores/redis.py 92.85% <0.00%> (+0.89%) ⬆️
sdk/python/feast/infra/online_stores/sqlite.py 97.59% <0.00%> (+1.20%) ⬆️
sdk/python/feast/registry.py 81.29% <0.00%> (+1.36%) ⬆️
sdk/python/feast/infra/local.py 92.15% <0.00%> (+1.96%) ⬆️
sdk/python/feast/infra/gcp.py 100.00% <0.00%> (+2.17%) ⬆️
sdk/python/feast/infra/aws.py 100.00% <0.00%> (+2.27%) ⬆️
sdk/python/feast/infra/online_stores/dynamodb.py 91.13% <0.00%> (+8.86%) ⬆️
... and 1 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 0d2179d...f88e12e. Read the comment docs.

@achals achals force-pushed the achal/dynamo-teardown branch from 327229b to ce2dac3 Compare July 8, 2021 04:20
@tsotnet
Copy link
Collaborator

tsotnet commented Jul 8, 2021

@achals Should we add the teardown method to the FeatureStore class instead of calling a private method on it?

@achals
Copy link
Member Author

achals commented Jul 8, 2021

@achals Should we add the teardown method to the FeatureStore class instead of calling a private method on it?

Definitely, that's what I wanted with #1696. I made this PR because I thought it may need to be landed sooner than that - if we're okay with me taking a few more hours I can fix that issue and update this PR to use the teardown method directly. What do you think?

@woop
Copy link
Member

woop commented Jul 8, 2021

@achals Should we add the teardown method to the FeatureStore class instead of calling a private method on it?

Definitely, that's what I wanted with #1696. I made this PR because I thought it may need to be landed sooner than that - if we're okay with me taking a few more hours I can fix that issue and update this PR to use the teardown method directly. What do you think?

sgtm

@feast-ci-bot feast-ci-bot added size/M and removed size/S labels Jul 8, 2021
@achals achals force-pushed the achal/dynamo-teardown branch from 99ba2f2 to 94388a8 Compare July 12, 2021 18:31
@achals achals requested a review from felixwang9817 July 12, 2021 21:08
Copy link
Collaborator

@felixwang9817 felixwang9817 left a comment

Choose a reason for hiding this comment

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

had one small nit, otherwise lgtm

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

@felixwang9817 felixwang9817 left a comment

Choose a reason for hiding this comment

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

/lgtm

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@feast-ci-bot feast-ci-bot merged commit 5aa9af2 into feast-dev:master Jul 13, 2021
@achals achals deleted the achal/dynamo-teardown branch July 13, 2021 23:28
SourdoughCat pushed a commit to SourdoughCat/feast that referenced this pull request Jul 16, 2021
* Teardown infrastructure after integration tests

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

* Add a teardown method in feature store

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

* fix import

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

* Rename var to tables

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

* Remove incorrect comment

Signed-off-by: Achal Shah <[email protected]>
Signed-off-by: CS <[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.

The FeatureStore class should expose a teardown method
6 participants