-
Notifications
You must be signed in to change notification settings - Fork 94
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
Use pytest 8 in CI #1822
Use pytest 8 in CI #1822
Conversation
mattwthompson
commented
Feb 5, 2024
- Tag issue being addressed
- Add tests
- Update docstrings/documentation, if applicable
- Lint codebase
- Update changelog
56f06bc
to
dae7240
Compare
Ha, I've gotten this error before but in a position where it didn't need to be fixed
|
5a1b105
to
8fc7d6e
Compare
93d6fc0
to
2470ca9
Compare
Splitting off some more discrete ideas which might be nice to have in the installed package before this works nicely, starting with |
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, this was clearly a ton of work all over the codebase and I really appreciate you hunting it all down. However in the current state I can't tell what's a temporary patch to get CI passing vs. what would actually be committed. Now that I see the green CI on this PR, could you modify it to be in its final state and update the PR text with any major subsequent steps that you'd plan to take after the review (otherwise I can't tell what's a temporary patch vs. a genuine oversight).
.github/workflows/conda.yml
Outdated
openff-toolkit-examples | ||
smirnoff-plugins=2024 | ||
pytest-xdist | ||
pytest-rerunfailures |
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.
(blocking) Could you expand on the reasoning for not putting these in conda_oe.yaml
? Especially with version pins this seems like something that may confuse us in the future. If these are outside of that file intentionally, it'd be good to leave a comment explaining why.
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 wanted to drop that file but instead I'm just reverting all of the "conda" tests
.github/workflows/conda.yml
Outdated
git fetch --tags | ||
git checkout tags/$LATEST_TAG | ||
git log -1 | cat |
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.
(blocking) This line seems essential to ensuring that the last release is being tested against its own tests, rather than the tests currently on main
. Could you clarify?
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.
As above
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.
Awesome, thanks for all the hard work and for iterating on this @mattwthompson. Please update the releasenotes before you merge!