-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix: Test latest_dataset is not none when bounding_box is None #498
Conversation
@@ -72,6 +72,9 @@ docker rm $container_name | |||
# delete the data | |||
rm -rf $SCRIPT_PATH/../data | |||
|
|||
# Add a slight delay because sometimes Docker does not seem ready after the rm. | |||
sleep 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
@@ -125,20 +125,7 @@ def test_gtfs_feeds_get(client: TestClient, mocker): | |||
subdivision_name="test_subdivision_name", | |||
municipality="test_municipality", | |||
) | |||
mock_bounding_box = json.dumps( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion]: to have better coverage, I suggest creating a test for the missing bounding box, leaving this test as it covers the bounding box's presence and structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually AFAIK the bounding box is not checked in this test.
I could add an assert for the bounding box in this test and create a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Summary:
Closes #440
Tested the conditions that lead to #431
Expected behavior:
These tests pass:
mobility-feed-api/api/tests/unittest/test_feeds.py
Line 114 in 8eee5c9
mobility-feed-api/api/tests/unittest/test_feeds.py
Line 199 in 8ebbffe
Testing tips:
To test I put back the code from PR #433 in api/src/feeds/impl/feeds_api_impl.py and verified that it failed.
Please make sure these boxes are checked before submitting your pull request - thanks!
./scripts/api-tests.sh
to make sure you didn't break anything