Skip to content

Generate python import tests in bazel#2096

Merged
garrettgu10 merged 9 commits intomainfrom
ggu/gen-import-tests
May 30, 2024
Merged

Generate python import tests in bazel#2096
garrettgu10 merged 9 commits intomainfrom
ggu/gen-import-tests

Conversation

@garrettgu10
Copy link
Contributor

@garrettgu10 garrettgu10 commented May 8, 2024 •
edited
Loading

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
dom96
dom96 reviewed May 9, 2024
View reviewed changes
Copy link
Contributor

@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!

@dom96 dom96 requested review from fhanau and mikea 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
mikea
mikea approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@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

@hoodmane hoodmane mentioned this pull request May 9, 2024
Closed
Base automatically changed from ggu/disk-cache-tests to main May 10, 2024 16:57
fhanau
fhanau reviewed May 10, 2024
View reviewed changes
@fhanau
Copy link
Contributor

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

hoodmane commented May 10, 2024

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

@garrettgu10
Copy link
Contributor Author

garrettgu10 commented May 10, 2024

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
Contributor Author

garrettgu10 commented May 11, 2024

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

@dom96
Copy link
Contributor

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 force-pushed the ggu/gen-import-tests branch from eb9823a to 37289e4 Compare May 13, 2024 21:40
@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 8 times, most recently from 7c4b6ed to 71b4eac Compare May 15, 2024 17:50
@garrettgu10 garrettgu10 force-pushed the ggu/gen-import-tests branch from 71b4eac to 7ec1d84 Compare May 15, 2024 17:55
@garrettgu10 garrettgu10 force-pushed the ggu/gen-import-tests branch 3 times, most recently from 1898cc1 to 60cf270 Compare May 29, 2024 01:20
@garrettgu10
Copy link
Contributor Author

garrettgu10 commented May 29, 2024 •
edited
Loading

@fhanau This PR is ready for review again after changes -- the tests are now configured to only run in the (linux) configuration, which was chosen because it was the fastest-running test pipeline. The idea is that the ~5 extra minutes spent on that pipeline would end up being spent on the other pipelines anyway.

Will require similar CI changes upstream.

fhanau
fhanau reviewed May 29, 2024
View reviewed changes
@garrettgu10 garrettgu10 force-pushed the ggu/gen-import-tests branch from 63af3c9 to 91a84f4 Compare May 30, 2024 17:01
@jasnell jasnell added the python Issues/PRs relating to Python Workers label May 30, 2024
@garrettgu10 garrettgu10 force-pushed the ggu/gen-import-tests branch from 91a84f4 to 816f051 Compare May 30, 2024 18:15
@garrettgu10 garrettgu10 merged commit cf6bee8 into main May 30, 2024
@hoodmane hoodmane mentioned this pull request Jun 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dom96 dom96 dom96 left review comments

@mikea mikea mikea approved these changes

@fhanau fhanau fhanau approved these changes

@hoodmane hoodmane Awaiting requested review from hoodmane

Assignees

No one assigned

Labels

python Issues/PRs relating to Python Workers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@garrettgu10 @fhanau @hoodmane @dom96 @mikea @jasnell