Skip to content
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

Generate python import tests in bazel #2096

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

garrettgu10
Copy link
Collaborator

@garrettgu10 garrettgu10 commented May 8, 2024

Prerequisite: #1970

Includes: #2108

This PR adds dozens of wd-tests generated by Bazel, based off a list generated by pyodide-build-scripts from the pyodide meta.yaml files and added onto pyodide_bucket.bzl.

Each test simply has the tested package listed as a requirement and a worker.py script that just imports a bunch of stuff from that package. I expect it will take a while to get all these tests passing but we do need them to pass eventually.

@garrettgu10 garrettgu10 requested review from dom96 and hoodmane May 8, 2024 21:35
@garrettgu10 garrettgu10 requested review from a team as code owners May 8, 2024 21:35
Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

I'd love @mikea and/or @fhanau to have a look and verify the way we're using bazel to generate these tests makes sense.

But otherwise LGTM. Thanks for implementing this, it makes our test coverage so much better!

build/pyodide_bucket.bzl Show resolved Hide resolved
@dom96 dom96 requested review from mikea and fhanau May 9, 2024 16:07
@garrettgu10 garrettgu10 changed the base branch from main to ggu/disk-cache-tests May 9, 2024 16:19
@garrettgu10 garrettgu10 force-pushed the ggu/gen-import-tests branch 2 times, most recently from 9ea1073 to e06ed59 Compare May 9, 2024 16:31
Copy link
Collaborator

@mikea mikea left a comment

Choose a reason for hiding this comment

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

build changes make sense mod couple nits

src/workerd/server/tests/python/import_tests.bzl Outdated Show resolved Hide resolved
src/workerd/server/tests/python/import_tests.bzl Outdated Show resolved Hide resolved
Base automatically changed from ggu/disk-cache-tests to main May 10, 2024 16:57
WORKSPACE Outdated Show resolved Hide resolved
@fhanau
Copy link
Collaborator

fhanau commented May 10, 2024

Code LGTM, I think auto-generating tests here will be really useful in improving python test coverage.
I hate to always bring up resource constraints but the ~50 tests being added all take between 20 - 50 seconds each – with the GitHub Actions runners being limited to 4 vCPUs each, that will slow down CI on every run making a code change. Is there any potential to optimize python-based tests?

@hoodmane
Copy link
Contributor

Could we gate the tests to only run on changes inside the pyodide folders?

@garrettgu10
Copy link
Collaborator Author

Could we gate the tests to only run on changes inside the pyodide folders?

Good idea, anyone know how to do that? 🫣

Maybe Bazel's cache could be useful if someone could point me to how to configure it 😮‍💨

@garrettgu10
Copy link
Collaborator Author

Is it possible to run the tests off a baseline snapshot as well? @hoodmane @dom96

@dom96
Copy link
Collaborator

dom96 commented May 13, 2024

Is it possible to run the tests off a baseline snapshot as well? @hoodmane @dom96

It's not something that we have set up, but it could be done. We have lots of tests using different kinds of snapshots in the EW repo, might be easier to do there.

@garrettgu10 garrettgu10 changed the title Generate import tests in bazel Generate python import tests in bazel May 13, 2024
@garrettgu10 garrettgu10 requested a review from a team as a code owner May 13, 2024 21:40
@garrettgu10 garrettgu10 force-pushed the ggu/gen-import-tests branch 3 times, most recently from b71f126 to 8edca96 Compare May 14, 2024 20:34
@garrettgu10 garrettgu10 force-pushed the ggu/gen-import-tests branch 5 times, most recently from 7c4b6ed to 71b4eac Compare May 15, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants