gh-149640: Add new GitHub action to test lazy_imports=all against test suite#151105
gh-149640: Add new GitHub action to test lazy_imports=all against test suite#151105brittanyrey wants to merge 6 commits into
Conversation
|
Thanks for the early review @StanFromIreland ! |
082f3f1 to
4dab14b
Compare
|
I'm not seeing my new test in checks, but it does show up as a failure in "All required checks pass". Not sure how to dig in from here. Edit: Fixed! |
hugovk
left a comment
There was a problem hiding this comment.
Thanks for this, looks good! I like the incremental method of tightening what is allowed to fail.
This workflow ensures that tests which currently work with lazy imports continue to work. If they regress, we get a failure, and need to fix it. This is good.
When we fix one of the excluded tests, we remove it from the list. Then is won't regress in the future. Also good!
What about if we "accidentally" fix an excluded test? We won't know it's been fixed, until someone checks, and then removes from the exclusion list.
Can we improve this? So for an "accidental" fix, the test also fails, but in a good way -- it tells you to also remove it from te exclude list, to prevent future regressions.
We have something similar for incrementally fixing Sphinx reference errors and tightening the allowed fails as we go: see https://github.com/python/cpython/blob/main/Doc/tools/.nitignore for the exclusion list, and https://github.com/python/cpython/blob/main/Doc/tools/check-warnings.py for the code that will fail_if_regression(), fail_if_new_news_nit() but also fail_if_improved().
Do you think something along these lines is worth it for lazy imports?
(Bonus points if we can think of a good .gitgnore/.nitignore pun for lazy_imports_all_exclude.txt, but not required :)
| path: ${{ env.CPYTHON_BUILDDIR }}/.hypothesis/examples/ | ||
|
|
||
| test-lazy-imports-all: | ||
| name: 'Lazy Imports All Enabled' |
There was a problem hiding this comment.
Some bikeshed suggestions:
| name: 'Lazy Imports All Enabled' | |
| name: 'Lazy imports' |
| name: 'Lazy Imports All Enabled' | |
| name: 'Lazy imports enabled' |
| name: 'Lazy Imports All Enabled' | |
| name: 'All lazy imports enabled' |
| run: make -j4 | ||
| - name: Display build info | ||
| run: make pythoninfo | ||
| - name: Verify lazy imports are fullly enabled |
There was a problem hiding this comment.
| - name: Verify lazy imports are fullly enabled | |
| - name: Verify lazy imports are fully enabled |
|
|
||
| jobs: | ||
| test-lazy-imports-all: | ||
| name: 'Lazy Imports All Enabled' |
There was a problem hiding this comment.
Match the previous choice:
| name: 'Lazy Imports All Enabled' | |
| name: 'Lazy imports' |
| name: 'Lazy Imports All Enabled' | |
| name: 'Lazy imports enabled' |
| name: 'Lazy Imports All Enabled' | |
| name: 'All lazy imports enabled' |
This commit adds a new action to run the CPython test suite with
lazy_imports=allset as suggested in this comment.#149640
-X lazy_imports=allmore? #149640