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
Improve bugfix tests #1339
Merged
Merged
Improve bugfix tests #1339
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The Dockerfile did not work as is, because the dependencies in requirements.txt are newer than the stretch-image with its python v3.5 can support/run. Use stable debian with the lts nodejs instead, plus had to add some libs to make the wheel build succeed. jsonschema v4 breaks things, so its version needs to be pinned until bravado is fixed [0]. [0] https://github.com/Yelp/bravado-core/pull/385/files#r731674447
Increased the version of pytest to make it work with py 3.10 [0]. The GET calls no longer return list but the object itself, fixed the tests and assertions to account for that. The tests did not account for the later added `allow_user_remove_domain` setting. And there were issues with missing and non-stopped patchers/mocks. Now all tests are at least passing. [0] pytest-dev/pytest#8540
PR #1089 is the culprit, as was already predicted in the review.
The `unit/apikey` directory is removed because it does not contain any tests. Same for `unit/test_decorators.py`. The `fixture` module is renamed to the special-name `conftest` [0] so they are available in all tests without the need to import them. With that in place, I removed all now unneeded or previously already unused imports from the tests. Also removed that wierd `sys.path` bit from `unit/zone/test_admin_apikey.py`, no idea what that was originally intended for. [0] https://docs.pytest.org/en/6.2.x/fixture.html#conftest-py-sharing-fixtures-across-multiple-files
Fix migration init_db 'id' Handle app context when needed Fix conftest fixtures Rearrange test Dockerfiles Hide DeprecationWarning during pytest execution Upgrade all python packages
@ymage thank you for your additional work on this! From what I can tell, these changes look good but I need to pull in another contributor for review before we accept the changes. |
Closed
AzorianMatt
added
mod / accepted
This request has been accepted
feature / update
Existing feature modification
and removed
mod / reviewing
This request is being reviewed
labels
Feb 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Heavily based on #1198 from @corubba (Many thanks)