-
Notifications
You must be signed in to change notification settings - Fork 32
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
Fix tests, run tests in CI #64
Conversation
This was incorrect on 3.10+
These are seemingly intentionally incompatible with pytest.
tests/test_basic.py
Outdated
class TestSysModules(CurrentVersionBase): | ||
# class TestSysModules(CurrentVersionBase): | ||
# # This relies on invocation in a clean python environment using unittest | ||
# # not using pytest. |
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 think I'd prefer to just remove these rather than comment them out if we're not going to be able to make them functional in this PR.
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.
Yep, I'm still messing around here -- I'm currently seeing whether it's feasible to update the older lists in CI, and I'll remove tests depending on how that pans out 🙂
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.
Done; I've rewritten the tests entirely (and enforced 100% coverage).
Okay, this should be good to go. To summarize:
|
# lint: | ||
# runs-on: ubuntu-latest | ||
# steps: | ||
# - uses: actions/checkout@v3 | ||
# - uses: actions/setup-python@v4 | ||
# with: | ||
# python-version: "3.10" | ||
# - name: lint | ||
# run: make lint INSTALL_EXTRA=lint |
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.
NB: Intentionally not enabled; will do in a separate PR.
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.
Reviewed in Conventional Comments style. Let me know if any don't make sense!
Closes #60.
Closes #55.
Closes #58.
Closes #32.