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.
CI, ENH: Check each minimum dependency is enforced in *.yaml and environment.yml #51189
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
CI, ENH: Check each minimum dependency is enforced in *.yaml and environment.yml #51189
Changes from 10 commits
4a15e02
f71a869
e4b90a2
527bd37
cbd87e5
c0c4ffc
e4789d2
1c8c8b4
6d6c391
7b3f50a
a4b4bdb
e2bb536
8397c94
3a73573
ed4827b
0968c10
6d30e63
f923b98
49e00c6
536450f
1431ae8
d2d5d73
1af6771
3b6123f
2c8c746
4af5291
6e8b0ef
5bd4908
78114fb
04dda2e
db0feae
4bd6e7a
bec1dd1
3c4e800
964e201
e89dd4d
47179f0
2c39a81
554ed85
b2a672b
985730f
0b25f3a
07c68b3
74965c2
635c055
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this is great, but it's getting quite complicated 😄 how about taking this part out to its own function (which accepts
yaml_file
as an argument), and then unit-testing that?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.
Sounds good! I've separated this portion of code into its own function. I'm writing unit tests now.
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.
thanks!
BTW if you notice some CI failures for some unrelated jobs, that's probably exactly what they are (i.e. unrelated)
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.
I wrote a new unit test file at
scripts/tests/test_validate_min_versions_in_sync.py
.It comes with 3 additional files under scripts/tests/:
The .toml file can be adjusted to change the baselines for the minimum dependencies. The
dummy_yaml_unmodified_file_1.yaml
can be adjusted to test different edge cases, however, thedummy_yaml_expected_file_1.yaml
should be changed accordingly to reflect the proper results for the previously mentioned file, since the two should be in sync as a testing set.I was thinking of creating more sets of expected/unmodified YAML files so that each set can test its own edge case like duplicates, same version, no version, range, etc. That way, each unit test can be smaller and more meaningful.
Currently, I think the dataset is too large to provide meaningful test assertion results.
@MarcoGorelli What are your thoughts? Feedback appreciated!
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.
nice! this isn't user-facing, so arguably doesn't need too extensive testing. but yes, smaller test files would make it easier to reason about what the script is doing
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.
I don't think this is necessary (the next line will overwrite the file)