| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Thanks for the early review @StanFromIreland ! |
Sorry, something went wrong.
|
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! |
Sorry, something went wrong.
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 :)
Sorry, something went wrong.
|
@hugovk I added the check to make sure the excluded list of tests still fail. Oddly, a lot of the ones that were failing locally for me & failing in the original issue stopped failing on CI? This was a bit surprising/confusing? cc: @JelleZijlstra Edit: It was a bug, addressing in commit. |
Sorry, something went wrong.
|
A few thoughts:
|
Sorry, something went wrong.
|
Also we need to add some mechanism (decorator?) to mark tests as expected to fail under lazy imports = all. My issue mentioned a few scenarios, like a test relying on the import side effect of creating a pyc file. |
Sorry, something went wrong.
We could use unittest.skipIf to check if lazy imports all is set. |
Sorry, something went wrong.
|
Addressed comments! Thanks for the review @StanFromIreland |
Sorry, something went wrong.
|
Thanks again @StanFromIreland. Let me know if there's anything else that needs updating. |
Sorry, something went wrong.
|
Alright, I think we're good! @StanFromIreland |
Sorry, something went wrong.
There was a problem hiding this comment.
This seems reasonable to me, at least till the list is empty, and we can debate again then.
Sorry, something went wrong.
There was a problem hiding this comment.
Looks good, thanks @brittanyrey! 🚀
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This commit adds a new action to run the CPython test suite with lazy_imports=all set as suggested in this comment.
#149640