Conversation
I mean that sounds like a big deal. |
|
Running pytest on the original test takes 55s. |
furtib
changed the title
furtib
changed the title
furtib
changed the title
f1af3ed to
db5885e
Compare
|
After rebase, it seems like it doesn't affect CI runtime. |
Add local = True
db5885e to
a97bd92
Compare
Szelethus
left a comment
Szelethus
left a comment
There was a problem hiding this comment.
It is kind of terrible how much boilerplate is needed to migrate this to bazel. And its especially a shame how much code we are seemingly duplicating from other tests. Is this absolutely necessary?
| parser.add_argument( | ||
| "--srcs", | ||
| nargs="*", | ||
| default=[], | ||
| help="Source files to copy into the workspace root", | ||
| ) |
There was a problem hiding this comment.
I rewrote the logic to not use temporary folders; now this is true.
| parser.add_argument( | ||
| "--expected_exit_code", | ||
| type=int, | ||
| default=0, | ||
| help="Expected exit code from the bazel command", | ||
| ) |
|
|
||
| external_test( | ||
| name = "compile_commands_isystem_test", | ||
| action = "build", |
There was a problem hiding this comment.
How integral is this to this test?
| parser.add_argument( | ||
| "--action", | ||
| required=True, | ||
| choices=["build", "test"], | ||
| help="Bazel action to run", | ||
| ) |
There was a problem hiding this comment.
This is technically used, but I'm sure there is a point.
There was a problem hiding this comment.
It's much better for extensibility if we, for whatever reason, want to extend this test.
| # Write empty WORKSPACE | ||
| write_file( | ||
| os.path.join(tmpdir, "WORKSPACE"), | ||
| "# This file is mandatory for old Bazel versions\n", |
There was a problem hiding this comment.
I mean, thats not the real explanation. We only support bazel 7 and up where its not a requirement, nor is it the default.
There was a problem hiding this comment.
I removed this, but a bit of correction.
Before Bazel 7.1.0, we still need WORKSPACE files for it to work. (But since the main WORKSPACE file was deleted in #270, I guess we don't support Bazel 7.0.0-7.0.2)
| action, | ||
| target, | ||
| "--experimental_cc_implementation_deps", | ||
| "--enable_bzlmod", |
There was a problem hiding this comment.
So we are testing here with bzlmod and workspace coexisting?
There was a problem hiding this comment.
I removed these flags; these were necessary for Bazel 6 and below.
| def cleanup(tmpdir): | ||
| """Shut down bazel server to release file locks.""" | ||
| subprocess.run( | ||
| ["bazel", "shutdown"], | ||
| cwd=tmpdir, | ||
| capture_output=True, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
Is this a common issue you are experiencing?
There was a problem hiding this comment.
This seems to speed up the time it takes for a test by about 15s.
So you mentioned in person this is likely due to bazelisk re-downloading bazel for each target? |
This branch has not been deployed
2 participants
Why:
We want all tests to be run with
bazel test //...What:
Addresses:
#210