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

inoculate test fixes #511

Merged
merged 2 commits into from
Jan 11, 2025
Merged

inoculate test fixes #511

merged 2 commits into from
Jan 11, 2025

Conversation

hawkw
Copy link
Owner

@hawkw hawkw commented Jan 11, 2025

Currently, the inoculate test command sets a timeout of 60 seconds, and
abruptly kills QEMU if it takes longer than that. This is no longer
nearly long enough to run our tests. This commit increases the timeout
to 1200 seconds (20 minutes). This is a long time, but our tests take a
while to run, and recall that this timeout applies to booting the kernel
and running all the tests, rather than being enforced on a per-test
basis.

We should probably make the timeout much smarter than just "kill the
QEMU process after 20 minutes", and instead do it only if we don't see
any interesting test output, or something. But, this is better than just
always failing test runs because we're stupid about timeouts, I guess.

I also fixed a bug where --nocapture disables the test protocol output parsing, breaking our detection of test failure/successes. Whoopsie.

Currently, the `--nocapture` flag to `cargo inoculate test` incorrectly
disables the processing of test output, so we can't actually detect that
tests are running. This is wrong. This commit makes it just print each
line of output to stdout, but still try to parse them.
@hawkw hawkw enabled auto-merge (rebase) January 11, 2025 19:27
hawkw added a commit that referenced this pull request Jan 11, 2025
Currently, the inoculate test command sets a timeout of 60 seconds, and
abruptly kills QEMU if it takes longer than that. This is no longer
nearly long enough to run our tests. This commit increases the timeout
to 1200 seconds (20 minutes). This is a long time, but our tests take a
while to run, and recall that this timeout applies to booting the kernel
*and running all the tests*, rather than being enforced on a per-test
basis.

We should probably make the timeout much smarter than just "kill the
QEMU process after 20 minutes", and instead do it only if we don't see
any interesting test output, or something. But, this is better than just
always failing test runs because we're stupid about timeouts, I guess.
@hawkw hawkw force-pushed the eliza/fix-inoculate-nocapture branch from 4330bc1 to 3380090 Compare January 11, 2025 19:33
Currently, the inoculate test command sets a timeout of 60 seconds, and
abruptly kills QEMU if it takes longer than that. This is no longer
nearly long enough to run our tests. This commit increases the timeout
to 1200 seconds (20 minutes). This is a long time, but our tests take a
while to run, and recall that this timeout applies to booting the kernel
*and running all the tests*, rather than being enforced on a per-test
basis.

We should probably make the timeout much smarter than just "kill the
QEMU process after 20 minutes", and instead do it only if we don't see
any interesting test output, or something. But, this is better than just
always failing test runs because we're stupid about timeouts, I guess.
@hawkw hawkw force-pushed the eliza/fix-inoculate-nocapture branch from 3380090 to a1249f4 Compare January 11, 2025 19:34
@hawkw hawkw disabled auto-merge January 11, 2025 19:46
@hawkw hawkw merged commit 7ee3c9e into main Jan 11, 2025
17 checks passed
hawkw added a commit that referenced this pull request Jan 11, 2025
Currently, the `--nocapture` flag to `cargo inoculate test` incorrectly
disables the processing of test output, so we can't actually detect that
tests are running. This is wrong. This commit makes it just print each
line of output to stdout, but still try to parse them.
@hawkw hawkw deleted the eliza/fix-inoculate-nocapture branch January 11, 2025 19:47
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.

1 participant