-
Notifications
You must be signed in to change notification settings - Fork 517
run container tests on CI #4421
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
Conversation
01716e9 to
d190cba
Compare
d190cba to
65698b1
Compare
65698b1 to
da08a21
Compare
aaf586d to
3ccf40a
Compare
3ccf40a to
2f53eb4
Compare
2f53eb4 to
86b5ce1
Compare
9fd8f76 to
4ea1d5b
Compare
ba785bf to
b610ade
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
19694f3 to
33f398d
Compare
|
The generated output of |
0f08162 to
76a11ca
Compare
fhanau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gpanders This looks like it should be several PRs – one making test changes, one adding container tests to CI. Easier to review that way.
The changes look good for the most part (only took a brief look so far). Do you have numbers for how much overhead this adds? As I understand you already only run this in the test-linux job, but keep in mind that the workers-sdk-test job depends on test-linux and runs after it, so test-linux may be in the critical path for CI duration already.
This PR only does the latter. The only changes to the tests are those that were required to get them to work when run by Bazel.
Using https://github.com/cloudflare/workerd/actions/runs/19862763581/job/56917088984 as a reference, the "Build and load container images step" adds 14s. "Bazel build" took 3m13s and "Bazel test" took 1m5s. Compared to https://github.com/cloudflare/workerd/actions/runs/19832826061/job/56822887495 (which is the last commit on main that this PR is based on right now), where "Bazel build" took 4m7s and "Bazel test" took 1m46s. The large difference is probably due to caching. But it doesn't seem like this PR adds any significant time. I'll add too that I'm working on this PR at @anonrig's request, who asked that this be done before #5491 is merged. I'll leave it up to you all whether or not you want to run these tests in CI, as long as we can unblock #5491. |
We definitely want these tests in CI. |
We use the rules_oci module to build an OCI image along with the very convenient js_image_layer rule provided by Aspect's rules_js.
This will prevent these targets from building in configurations that can't (or don't) run container tests.
47370ef to
bd8efb4
Compare
fhanau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise
Let's run container tests on CI