tests: Keep testdir only when test fails #2006
Conversation
The current runtest.py -k/--keep option makes the test directory in /tmp move into the tests directory for later reference. But it tends to keep too many test_* directories so it'd be better to keep only the test directories of failed tests. Signed-off-by: Honggyu Kim <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR refines the --keep behavior so that only directories for failed tests are preserved, and it writes each failure’s diff into diff.txt.
- Introduces
TEST_NORMALto group non-failure exit codes - Updates
__del__to move only failed-test dirs and record diffs - Adjusts retention logic in
run_single_caseandprint_test_resultto useTEST_NORMAL
Comments suppressed due to low confidence (1)
tests/runtest.py:827
- [nitpick] The variable name
difis unclear—consider renaming it todiff_textor similar to improve readability.
ret, dif = tc.run(case, cflags, arg.diff, timeout)
| ret, dif = tc.run(case, cflags, arg.diff, timeout) | ||
| ret = tc.postrun(ret) | ||
| tc.ret = ret | ||
| tc.dif = dif |
There was a problem hiding this comment.
The local variable dif may be uninitialized if the branch that sets it wasn’t executed, causing an UnboundLocalError. Consider initializing dif (e.g., to an empty string) before the conditional block.
Copilot uses AI. Check for mistakes.
| if r not in TestBase.TEST_NORMAL: | ||
| ftests.write(output) | ||
| ftests.flush() | ||
| break | ||
|
|
||
| if arg.quiet: | ||
| for r in result: | ||
| if r not in normal: | ||
| if r not in TestBase.TEST_NORMAL: |
There was a problem hiding this comment.
Here r is a tuple (ret, dif), but you’re comparing it against a list of ints, so this condition will always be true. You likely meant to check the return code, e.g., if r[0] not in TestBase.TEST_NORMAL:.
Copilot uses AI. Check for mistakes.
| if r not in TestBase.TEST_NORMAL: | ||
| ftests.write(output) | ||
| ftests.flush() | ||
| break | ||
|
|
||
| if arg.quiet: | ||
| for r in result: | ||
| if r not in normal: | ||
| if r not in TestBase.TEST_NORMAL: |
There was a problem hiding this comment.
Same issue as above: r is a (ret, dif) tuple but is compared to ints. Use r[0] to compare the return code against TEST_NORMAL.
Copilot uses AI. Check for mistakes.
| TEST_NONZERO_RETURN = -6 | ||
| TEST_SKIP = -7 | ||
| TEST_SUCCESS_FIXED = -8 | ||
| TEST_NORMAL = [TEST_SUCCESS, TEST_SUCCESS_FIXED, TEST_SKIP] |
There was a problem hiding this comment.
[nitpick] TEST_NORMAL is defined as a list but never modified; using a tuple (e.g., (...,)) would better convey immutability for a constant grouping.
| TEST_NORMAL = [TEST_SUCCESS, TEST_SUCCESS_FIXED, TEST_SKIP] | |
| TEST_NORMAL = (TEST_SUCCESS, TEST_SUCCESS_FIXED, TEST_SKIP) |
Copilot uses AI. Check for mistakes.
|
I think it sometimes needs to keep successful test directories. To get the failed tests, you can run it again with options to limit the test cases without this change. |
|
I agree. But we may need to have a way to opt in and out such cases. I saw many random failures in AArch64 tests. |
2 participants
The current runtest.py -k/--keep option makes the test directory in /tmp move into the tests directory for later reference.
But it tends to keep too many test_* directories so it'd be better to keep only the test directories of failed tests.