Skip to content

gh-149640: Add new GitHub action to test lazy_imports=all against test suite#151105

Open
brittanyrey wants to merge 6 commits into
python:mainfrom
brittanyrey:lazy-imports-all-ci
Open

gh-149640: Add new GitHub action to test lazy_imports=all against test suite#151105
brittanyrey wants to merge 6 commits into
python:mainfrom
brittanyrey:lazy-imports-all-ci

Conversation

@brittanyrey

@brittanyrey brittanyrey commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

This commit adds a new action to run the CPython test suite with lazy_imports=all set as suggested in this comment.

#149640

Comment thread .github/workflows/lazy-imports-all.yml Outdated
Comment thread .github/workflows/reusable-test-lazy-imports-all.yml
@brittanyrey

Copy link
Copy Markdown
Contributor Author

Thanks for the early review @StanFromIreland !

@brittanyrey brittanyrey force-pushed the lazy-imports-all-ci branch from 082f3f1 to 4dab14b Compare June 9, 2026 18:01
@brittanyrey brittanyrey marked this pull request as ready for review June 9, 2026 19:03
@StanFromIreland StanFromIreland added skip news infra CI, GitHub Actions, buildbots, Dependabot, etc. labels Jun 9, 2026
@brittanyrey

brittanyrey commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

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!

Comment thread .github/workflows/reusable-test-lazy-imports-all.yml Outdated
@brettcannon brettcannon removed their request for review June 10, 2026 19:59

@hugovk hugovk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some bikeshed suggestions:

Suggested change
name: 'Lazy Imports All Enabled'
name: 'Lazy imports'
Suggested change
name: 'Lazy Imports All Enabled'
name: 'Lazy imports enabled'
Suggested change
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: Verify lazy imports are fullly enabled
- name: Verify lazy imports are fully enabled


jobs:
test-lazy-imports-all:
name: 'Lazy Imports All Enabled'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Match the previous choice:

Suggested change
name: 'Lazy Imports All Enabled'
name: 'Lazy imports'
Suggested change
name: 'Lazy Imports All Enabled'
name: 'Lazy imports enabled'
Suggested change
name: 'Lazy Imports All Enabled'
name: 'All lazy imports enabled'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review infra CI, GitHub Actions, buildbots, Dependabot, etc. skip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants