Migrate selector tests to pytest #10281
Closed
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
We have a globally available `manifest` fixture in our unit tests. In the coming commits we're going to add tests to the file which use the gloablly available `manifest` fixture. Prior to this commit, the locally defined `manifest` fixture was taking precidence. To get around this, the easiest solution was to rename the locally defined fixture. I had tried to isolate the locally defined fixture by moving it, and the relevant tests to a test class like `TestNodeSelector`. However because of _how_ the relevant tests were parameterized, this proved difficult. Basically for readability we define a variable which holds a list of all the parameterization variables. By moving to a test class, the definition of the variables would have had to be defined directly in the parameterization macro call. Although possible, it made the readability slighty worse. It might be worth doing anyway in the long run, but instead I used a less heavy handed alternative (already described)
…in unit tests The `Compiler.compile` method accesses `self.config.args.which`. The `config` is the `RuntimeConfig` the `Compiler` was instantiated with. Our `runtime_config` fixture was being instatiated with an empty dict for the `args` property. Thus a `which` property of the args wasn't being made avaiable, and if `compile` was run a runtime error would occur. To solve this, we've begun instantiating the args from the global flags via `get_flags()`. This works because we ensure the `set_test_flags` fixture is run first which calls `set_from_args`.
… use pytesting methodology
We had some tests in `test_selector.py::GraphTest` that didn't add anything ontop of what was already being tested else where in the file except the parsing of models. However, the tests in `test_parser.py::ModelParserTest` cover everything being tested here (and then some). Thus these tests in `test_selector.py::GraphTest` are unnecessary and can be deleted.
There was a test `test__partial_parse` in `test_selector.py` which tested the functionality of `is_partial_parsable` of the `ManifestLoader`. This doesn't really make sense to exist in `test_selector.py` where we are testing selectors. We test the `ManifestLoader` class in `test_manifest.py` which seemed like a more appropriate place for the test. Additionally we renamed the test to `test_is_partial_parsable_by_version` to more accurately describe what is being tested.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10281 +/- ##
==========================================
- Coverage 88.73% 88.67% -0.06%
==========================================
Files 180 180
Lines 22474 22495 +21
==========================================
+ Hits 19942 19948 +6
- Misses 2532 2547 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Closing this pull request as I don't want anyone to interact with a comment made by user simulified. It overtakes the page and displays a flashing image which contains an invite code to a discord server. |
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.
resolves #9868
Problem
In #9868 we wanted to ensure we write, and can write, some selector tests. After creating the ticket, we actually found some selector tests while we were reorganizing tests. Thus #9868 became about converting these tests to use pytest.
Solution
Converted selector tests to use pytest, showing that we can write selector tests
Checklist