-
-
Notifications
You must be signed in to change notification settings - Fork 106
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
[DNM] Add documentation for hypothetical pre-timeout scripts #2018
base: main
Are you sure you want to change the base?
Changes from 4 commits
1a5df32
0e7f441
aacf019
a107acf
4e5a087
6fa63e6
b2bc337
a9103e6
9b68ab5
823eb6e
ed1df40
38dac56
121f268
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
--- | ||
icon: material/script-text | ||
--- | ||
|
||
# Scripts | ||
|
||
Nextest supports running _scripts_ when certain events occur during a test run. Scripts can be scoped to particular tests via [filtersets](../filtersets/index.md). | ||
|
||
Nextest currently recognizes two types of scripts: | ||
|
||
* [_Setup scripts_](setup.md), which execute at the start of a test run. | ||
* [_Pre-timeout scripts_](pre-timeout.md), which execute before nextest terminates a test that has exceeded its timeout. | ||
|
||
Scripts are configured in two parts: _defining scripts_, and _specifying rules_ for when they should be executed. | ||
|
||
## Defining scripts | ||
|
||
Scripts are defined using the top-level `script.<type>` configuration. | ||
|
||
For example, to define a setup script named "my-script", which runs `my-script.sh`: | ||
|
||
```toml title="Setup script definition in <code>.config/nextest.toml</code>" | ||
[script.setup.my-script] | ||
command = 'my-script.sh' | ||
# Additional options... | ||
``` | ||
|
||
See [_Defining setup scripts_](setup.md#defining-setup-scripts) for the additional options available for configuring setup scripts. | ||
|
||
To instead define a pre-timeout script named "my-script", which runs `my-script.sh`: | ||
|
||
```toml title="Pre-timeout script definition in <code>.config/nextest.toml</code>" | ||
[script.pre-timeout.my-script] | ||
command = 'my-script.sh' | ||
# Additional options... | ||
``` | ||
|
||
See [_Defining pre-timeout scripts_](pre-timeout.md#defining-pre-timeout-scripts) for the additional options available for configuring pre-timeout scripts. | ||
|
||
### Command specification | ||
|
||
All script types support the `command` option, which specifies how to invoke the script. Commands can either be specified using Unix shell rules, or as a list of arguments. In the following example, `script1` and `script2` are equivalent. | ||
|
||
```toml | ||
[script.<type>.script1] | ||
command = 'script.sh -c "Hello, world!"' | ||
|
||
[script.<type>.script2] | ||
command = ['script.sh', '-c', 'Hello, world!'] | ||
``` | ||
|
||
### Timeouts | ||
|
||
All script types support the following timeout options: | ||
|
||
- **`slow-timeout`**: Mark a script [as slow](../features/slow-tests.md) or [terminate it](../features/slow-tests.md#terminating-tests-after-a-timeout), using the same configuration as for tests. By default, scripts are not marked as slow or terminated (this is different from the slow timeout for tests). | ||
- **`leak-timeout`**: Mark scripts [leaky](../features/leaky-tests.md) after a timeout, using the same configuration as for tests. By default, the leak timeout is 100ms. | ||
|
||
|
||
```toml title="Script definition with timeouts" | ||
[script.<type>.my-script] | ||
command = 'script.sh' | ||
slow-timeout = { period = "60s", terminate-after = 2 } | ||
leak-timeout = "1s" | ||
``` | ||
|
||
### Namespacing | ||
|
||
Script names must be unique across all script types. | ||
|
||
This means that you cannot use the same name for a setup script and a pre-timeout script: | ||
|
||
```toml title="Pre-timeout script definition in <code>.config/nextest.toml</code>" | ||
[script.setup.my-script] | ||
command = 'setup.sh' | ||
|
||
# Reusing the `my-script` name for a pre-timeout script is NOT permitted. | ||
[script.pre-timeout.my-script] | ||
command = 'pre-timeout.sh' | ||
``` | ||
|
||
## Specifying rules | ||
|
||
In configuration, you can create rules for when to use scripts on a per-profile basis. This is done via the `profile.<profile-name>.scripts` array. For example, you can configure a setup script that generates a database if tests from the `db-tests` package, or any packages that depend on it, are run. | ||
|
||
```toml title="Basic rules" | ||
[[profile.default.scripts]] | ||
filter = 'rdeps(db-tests)' | ||
setup = 'db-generate' | ||
``` | ||
|
||
(This example uses the `rdeps` [filterset](../filtersets/index.md) predicate.) | ||
|
||
Scripts can also filter based on platform, using the rules listed in [_Specifying platforms_](../configuration/specifying-platforms.md): | ||
|
||
```toml title="Platform-specific rules" | ||
[[profile.default.scripts]] | ||
platform = { host = "cfg(unix)" } | ||
setup = 'script1' | ||
``` | ||
|
||
A set of scripts can also be specified. All scripts in the set will be executed. | ||
|
||
```toml title="Multiple setup scripts" | ||
[[profile.default.scripts]] | ||
filter = 'test(/^script_tests::/)' | ||
setup = ['script1', 'script2'] | ||
``` | ||
|
||
Executing pre-timeout scripts follows the same pattern. For example, you can configure a pre-timeout script for every test that contains `slow` in its name. | ||
|
||
```toml title="Basic pre-timeout rules" | ||
[[profile.default.scripts]] | ||
filter = 'test(slow)' | ||
pre-timeout = 'capture-backtrace' | ||
``` | ||
|
||
A single rule can specify any number of setup scripts and any number of pre-timeout scripts. | ||
|
||
```toml title="Combination rules" | ||
[[profile.default.scripts]] | ||
filter = 'test(slow)' | ||
setup = ['setup-1', 'setup-2'] | ||
pre-timeout = ['pre-timeout-1', 'pre-timeout-2'] | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
--- | ||
icon: material/timer-sand-empty | ||
status: experimental | ||
--- | ||
|
||
<!-- md:version 0.9.59 --> | ||
|
||
# Pre-timeout scripts | ||
|
||
!!! experimental "Experimental: This feature is not yet stable" | ||
|
||
- **Enable with:** Add `experimental = ["pre-timeout-scripts"]` to `.config/nextest.toml` | ||
- **Tracking issue:** [#TODO](https://github.com/nextest-rs/nextest/issues/TODO) | ||
|
||
|
||
Nextest runs *pre-timeout scripts* before terminating a test that has exceeded | ||
its timeout. | ||
|
||
Pre-timeout scripts are useful for automatically collecting backtraces, logs, etc. that can assist in debugging why a test is slow or hung. | ||
|
||
## Defining pre-timeout scripts | ||
|
||
Pre-timeout scripts are defined using the top-level `script.pre-timeout` configuration. For example, to define a script named "my-script", which runs `my-script.sh`: | ||
|
||
```toml title="Script definition in <code>.config/nextest.toml</code>" | ||
[script.pre-timeout.my-script] | ||
command = 'my-script.sh' | ||
``` | ||
|
||
See [_Defining scripts_](index.md#defining-scripts) for options that are common to all scripts. | ||
|
||
Pre-timeout scripts do not support additional configuration options. | ||
|
||
Notably, pre-timeout scripts always capture stdout and stderr. Support for not capturing stdout and stderr may be added in the future in order to support use cases like interactive debugging of a hung test. | ||
|
||
### Example | ||
|
||
To invoke GDB to dump backtraces before a hanging test is terminated: | ||
|
||
```toml title="Advanced pre-timeout script definition" | ||
[script.pre-timeout.gdb-dump] | ||
command = ['sh', '-c', 'gdb -p $NEXTEST_PRE_TIMEOUT_TEST_PID -batch -ex "thread apply all backtrace"'] | ||
# TODO options | ||
``` | ||
|
||
## Specifying pre-timeout script rules | ||
|
||
See [_Specifying rules_](index.md#specifying-rules). | ||
|
||
## Pre-timeout script execution | ||
|
||
A given pre-timeout script _S_ is executed when the current profile has at least one rule where the `platform` predicates match the current execution environment, the script _S_ is listed in `pre-timeout`, and a test matching the `filter` has reached its configured timeout. | ||
|
||
Pre-timeout scripts are executed serially, in the order they are defined (_not_ the order they're specified in the rules). If any pre-timeout script exits with a non-zero exit code, an error is logged but the test run continues. | ||
|
||
Nextest will proceed with graceful termination of the test only once the pre-timeout script terminates. See [_How nextest terminates tests_](#defining-pre-timeout-scripts). If the pre-timeout script itself is slow, nextest will apply the same termination protocol to the pre-timeout script. | ||
|
||
The pre-timeout script is not responsible for terminating the test process, but it is permissible for it to do so. | ||
Comment on lines
+58
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Beginning to address this question:
and this one:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I may need to get further into the code to really understand the complexity here, but it's not immediately obvious to me why we need to couple the script and test lifetimes together or put the processes in the same pgrp. Naively it seems straightforward to just apply the graceful termination logic to the pre-timeout script and then move on to apply the graceful termination logic to the test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's definitely possible to do that, but my concern is just that it blows up the state machine even further. The state machine for an individual test already has on the order of 40-50 states today once you consider all the combinations of Adding a "start script -> read stdout/stderr from script -> wait on script + test to exit -> check for the script leaking handles" already blows it up quite a bit, though the benefits outweigh the complexity. Adding "SIGTERM to script -> maybe SIGKILL -> script done -> SIGTERM to test -> maybe SIGKILL -> test done" adds even more states to the state machine, and I'm not sure this pays its way compared to killing the script and the test at the same time. But, I agree that this decision can be made based on how complex the implementation gets. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It wasn't terribly difficult to add a
|
||
|
||
Nextest executes pre-timeout scripts with the same working directory as the test and sets the following variables in the script's environment: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a clause here specifying the CWD:
|
||
|
||
* **`NEXTEST_PRE_TIMEOUT_TEST_PID`**: the ID of the process running the test. | ||
* **`NEXTEST_PRE_TIMEOUT_TEST_NAME`**: the name of the running test. | ||
* **`NEXTEST_PRE_TIMEOUT_TEST_BINARY_ID`**: the ID of the binary in which the test is located. | ||
* **`NEXTEST_PRE_TIMEOUT_TEST_BINARY_ID_PACKAGE_NAME`**: the package name component of the binary ID. | ||
* **`NEXTEST_PRE_TIMEOUT_TEST_BINARY_ID_NAME`**: the name component of the binary ID, if known. | ||
* **`NEXTEST_PRE_TIMEOUT_TEST_BINARY_ID_KIND`**: the kind component of the binary ID, if known. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sunshowers would love your input on whether these are the right names for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'd match the filterset DSL. I'd change:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Makes sense to me! |
||
|
||
<!-- TODO: a protocol for writing script logs to a file and telling nextest to attach them to JUnit reports? --> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@sunshowers are you comfortable deferring this to future work? I'd like to try to keep the initial implementation as focused as possible, and this bit seems a hunk of work that's easy to split off. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, fine to defer to the future. I just want to make sure we don't box ourselves into making this impossible in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting to address this:
I was thinking to keep things simple for the initial implementation and force that stdout/stderr are captured. @sunshowers does that make sense to you?
Flagging that I haven't forgotten about this! I think I'll need to get a little further along in the implementation before I can answer this question, though. Happy to take any advice you have in the meantime, though, @sunshowers, if there's a particular UX you're imagining!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is ok, keep it simple for now.
So there's several points to surface user information here.
At the moment the test has timed out:
TERMINATING
line:nextest/nextest-runner/src/reporter/displayer/imp.rs
Lines 347 to 364 in e595f3f
-
is in the same column as]
)nextest/nextest-runner/src/reporter/displayer/snapshots/nextest_runner__reporter__displayer__imp__windows_tests__stack_violation_code.snap
Line 6 in e595f3f
Once the script is done executing:
We should print the test's output, as well as the script's output, with the two being clearly distinguished from each other.
In information queries: (i.e.
t
, or ctrl-t/SIGINFO where available):We should say something like "terminating due to timeout" -> "running pre-timeout script foo", and carry around all the details for foo just like we would for any other unit.
To do this we'd need to expand the terminating state to carry this information, I think:
nextest/nextest-runner/src/reporter/events.rs
Lines 1023 to 1045 in e595f3f
Maybe in an
Option<Box<T>>
field.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! I ended up with a slightly different approach than you proposed. Instead of adding an optional pre-timeout field to the
Terminating
state, I added a new top-levelUnitState::PreTimeout
state, which itself contains ascript_state: UnitState
field. This allows the pre-timeout script itself to transition between the standard unit states (running, slow, terminating, exited) with minimal code duplication.Here's how the output looks in this initial implementation.
Basic statuses:
A test with an activated pre-timeout script transitions like so:
SLOW
)PRETMT
)PRETMT SLOW
)PRETMT PASS
/PRETMT FAIL
, followed by the pre-timeout script's stdout/stderr)TERMINATING
)TIMEOUT
, followed by the test's stdout/stderr)If you request status while the pre-timeout script is executing, you see something like this:
I'm by no means married to any of this. It's just a result of my initial attempt to balance "clear to end users" and "straightforward to implement."