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

i#6558, i#6127: Set CTest timeouts for all tests #6563

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

derekbruening
Copy link
Contributor

@derekbruening derekbruening commented Jan 12, 2024

Before, we relied on drrun -s for all test suite timeouts except for runcmp tests where we set a CTest timeout. This resulted in the default 10 minute CTest timeout for all tests, which was the only timeout for runall tests and caused long suite times on the AArch64 machine which accidentally had no ptrace privileges (#5740, #6558,

Here, we set the CTest time for runall in addition to runcmp, and for all other tests with no timeout specified (which are presumably relying on drrun -s) we set a timeout of the drrun timeout plus 30 seconds.

Tested on the attach test:

Before:

123: Test timeout computed to be: 1500

Now:

$ echo 1 | sudo tee /proc/sys/kernel/yama/ptrace_scope; /usr/bin/time ctest -V -R client.attach_test; echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope
...
    Start 121: code_api|client.attach_test
...
121: Test timeout computed to be: 90
1/1 Test #121: code_api|client.attach_test ......***Timeout  90.11 sec
...
The following tests FAILED:
	121 - code_api|client.attach_test (Timeout)
...
Command exited with non-zero status 8
1.13user 0.80system 1:30.14elapsed 2%CPU (0avgtext+0avgdata 13196maxresident)k

The property being set on more tests was confirmed on debug x86-64:
Before:

$ grep -c TIMEOUT suite/tests/CTestTestfile.cmake
94

After:

$ grep -c TIMEOUT suite/tests/CTestTestfile.cmake
463

There seem to still be a few missing the property: the ones that don't go through suite/. There were other efforts to avoid hangs on those such as PR #6137.

Fixes #6127
Issue: #6127, #6558, #5740

Before, we relied on drrun -s for all test suite timeouts except for
runcmp tests where we set a CTest timeout.  This resulted in the
default 10 minute CTest timeout for all tests, which was the only
timeout for runall tests and caused long suite times on the AArch64
machine which accidentally had no ptrace privileges (#5740, #6558,

Here, we set the CTest time for runall in addition to runcmp, and for
all other tests with no timeout specified (which are presumably
relying on drrun -s) we set a timeout of the drrun timeout plus 30
seconds.

Tested on the attach test:

Before:
```
123: Test timeout computed to be: 1500
```
Now:
```
$ echo 1 | sudo tee /proc/sys/kernel/yama/ptrace_scope; /usr/bin/time ctest -V -R client.attach_test; echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope
...
    Start 121: code_api|client.attach_test
...
121: Test timeout computed to be: 90
1/1 Test #121: code_api|client.attach_test ......***Timeout  90.11 sec
...
The following tests FAILED:
	121 - code_api|client.attach_test (Timeout)
...
Command exited with non-zero status 8
1.13user 0.80system 1:30.14elapsed 2%CPU (0avgtext+0avgdata 13196maxresident)k
```

Fixes #6127
Issue: #6127, #6558, #5740
@derekbruening
Copy link
Contributor Author

amd32-bit failure is #6417 (getting to the point where we should either remove it from the set of jobs or have a Fixit to get it green...)

@xdje42
Copy link
Contributor

xdje42 commented Jan 16, 2024

I don't yet have an opinion on this patch, but wouldn't a better fix for the yama ptrace_scope tests (eg, #6558) be to have them fail fast instead of timing out?

@derekbruening
Copy link
Contributor Author

I don't yet have an opinion on this patch, but wouldn't a better fix for the yama ptrace_scope tests (eg, #6558) be to have them fail fast instead of timing out?

This is not trying to fix the attach/detach tests: it is ensuring the timeout value of 90s we thought we had set for all tests is actually set for all tests instead of having some tests fall back to the general CTest default of 10 minutes which we never want.

@derekbruening
Copy link
Contributor Author

(Generally it's better to use the review comments to have an actual threaded conversation rather than trying to do so in these top-level comments -- please use the review interface.)

@xdje42
Copy link
Contributor

xdje42 commented Jan 16, 2024

JTBC, Is the rule that review comments shall be used for all comments?
Or put another way, when would one use the top level comments?

@derekbruening
Copy link
Contributor Author

JTBC, Is the rule that review comments shall be used for all comments? Or put another way, when would one use the top level comments?

IMHO, in a PR they are only useful for noting flaky failures. There is no threading or marking of "resolved" to gate submission so they are not useful for much else.

@derekbruening derekbruening removed the request for review from ivankyluk January 16, 2024 21:36
@derekbruening derekbruening merged commit 8ea7016 into master Jan 16, 2024
14 of 15 checks passed
@derekbruening derekbruening deleted the i6127-set-timeout-for-all branch January 16, 2024 21:51
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.

Speed up test suite to <20 minutes from >30 minutes
2 participants