DISCO-3182 Remove duplicated fixtures #773
Merged
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
misaniwere
requested changes
Jan 29, 2025
There was a problem hiding this comment.
Thanks for cleaning this up! There are a few things left to cleanup:
gcs_blob_mockwas previously a fixture that returned a function and there's still traces of that in the code where it is being used as a function. e.ggcs_blob_mock(blob_json, "manifest.json")- There are tests that are are defining mock objects when we have access to
gcs_blob_mock, for example, this line can be changed togcs_bucket_mock.get_blob.return_value = gcs_blob_mock - There are multiple test methods in manifest and top picks folders where we're passing in fixtures and then not using them. for example, here
These are just a few instances where we can refactor/clean up fixtures but there are more opportunities to tighten up the code here. Let me know if you need more clarification, happy to hop on a call :)
gruberb
force-pushed
the
DISCO-3182-remove-dups
branch
from
40ce58d to
c470a03
Compare
gruberb
force-pushed
the
DISCO-3182-remove-dups
branch
from
c470a03 to
8b0f93a
Compare
misaniwere
approved these changes
Jan 29, 2025
| ) -> None: | ||
| """Test that the get_file method returns manifest data.""" | ||
| manifest_remote_filemanager.gcs_client = MagicMock() | ||
| manifest_remote_filemanager.gcs_client.get_file_by_name.return_value = gcs_blob_mock | ||
| gcs_blob_mock.download_as_text.return_value = blob_json | ||
|
|
||
| get_file_result_code, result = manifest_remote_filemanager.get_file() |
There was a problem hiding this comment.
nit: can we add a success check for get_file_result_code or change to _ since it's an unused variable
gruberb
force-pushed
the
DISCO-3182-remove-dups
branch
from
8b0f93a to
d7e5410
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
References
JIRA: DISCO-3182
Description
Remove duplicated test fixtures.
PR Review Checklist
Put an
xin the boxes that apply[DISCO-####], and has the same title (if applicable)[load test: (abort|skip|warn)]keywords are applied to the last commit message (if applicable)