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

BazelTestRunner: exit with 137 on OOMs #24436

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

saraadams
Copy link
Contributor

Motivation: Make it easier to detect whether a test failed due to an OutOfMemoryException

Current behavior:
The test runner exited with a code 1 on any test failure. This can make it hard to programmatically differentiate whether a test failed due to an actual test failure, or because the test was aborted due to an OutOfMemoryException.

Proposed change:
If a run of a test suite fails, the failures are checked for any OOMs. If at least one failure was caused by an OOM, the program exits with a code 137, rather than 1. Additionally, this is also the exit code if the test runner itself OOMs.

This distinction can help with automatically detecting OOMs and then retrying the execution with more memory available, and is especially helpful in the context of remote execution, where the action may be retried on a larger executor.

Motivation: Make it easier to detect whether a test failed due to an
OutOfMemoryException

Current behavior:
The test runner exited with a code 1 on any test failure.
This can make it hard to programmatically differentiate whether a test
failed due to an actual test failure, or because the test was aborted
due to an OutOfMemoryException.

Proposed change:
If a run of a test suite fails, the failures are checked for any OOMs.
If at least one failure was caused by an OOM, the program exits with a
code 137, rather than 1. Additionally, this is also the exit code if
the test runner itself OOMs.

This distinction can help with automatically detecting OOMs and then
retrying the execution with more memory available, and is especially
helpful in the context of remote execution, where the action may be
retried on a larger executor.
@github-actions github-actions bot added team-Rules-Java Issues for Java rules awaiting-review PR is awaiting review from an assigned reviewer labels Nov 21, 2024
@hvadehra
Copy link
Member

This seems reasonable to me. Any chance we could add a test for this?

@saraadams
Copy link
Contributor Author

This seems reasonable to me. Any chance we could add a test for this?

I had looked, but didn't see where I could easily add a test. Do you have guidance on where/how I could add a test for this?

@saraadams
Copy link
Contributor Author

Added a test - is that the right way to do it?

@hvadehra
Copy link
Member

Thanks for adding the tests. Could you maybe separate out the new test into a different class? It would be preferable to not modify existing tests so we can be sure we're not inadvertently affecting something else.

@saraadams
Copy link
Contributor Author

Thanks for adding the tests. Could you maybe separate out the new test into a different class? It would be preferable to not modify existing tests so we can be sure we're not inadvertently affecting something else.

Just so I head in the right direction:

  1. Do you want me to add a separate Java test class for the OOM test case? If I keep the current change and reverted the changes to junit4_testbridge_integration_tests.sh that should still pass.
  2. Do you want me to add a separate test case in the existing src/java_tools/junitrunner/javatests/com/google/testing/junit/runner/junit4_testbridge_integration_tests.sh or do you want me to add a separate .sh? It looks to me like adding a separate test function would be sufficient.

@hvadehra
Copy link
Member

A separate java test class and new test case in the same sh file SG.

@saraadams
Copy link
Contributor Author

A separate java test class and new test case in the same sh file SG.

PTAL.

@hvadehra
Copy link
Member

Thanks!

@hvadehra hvadehra added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally team-Rules-Java Issues for Java rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants