From 1a5df32c56558d10d1c43a750edfc39e62c90126 Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Thu, 26 Dec 2024 00:40:13 -0500 Subject: [PATCH 01/13] [DNM] Add documentation for hypothetical pre-timeout scripts In the spirit of documentation driven development, this commit adds docs for the pre-timeout scripts proposed in #1906. I'll start prototyping an implementation soon. In the meantime, I figured we could start shaking out the major design questions by reviewing what's proposed in the documentation here. --- .../{setup-scripts.md => scripts.md} | 68 +++++++++++++++---- 1 file changed, 54 insertions(+), 14 deletions(-) rename site/src/docs/configuration/{setup-scripts.md => scripts.md} (59%) diff --git a/site/src/docs/configuration/setup-scripts.md b/site/src/docs/configuration/scripts.md similarity index 59% rename from site/src/docs/configuration/setup-scripts.md rename to site/src/docs/configuration/scripts.md index ea27db506a0..822d1d51d11 100644 --- a/site/src/docs/configuration/setup-scripts.md +++ b/site/src/docs/configuration/scripts.md @@ -3,25 +3,34 @@ icon: material/ray-start-arrow status: experimental --- -# Setup scripts +# Scripts -!!! experimental "Experimental: This feature is not yet stable" +!!! experimental "Experimental: Setup scripts are not yet stable" - **Enable with:** Add `experimental = ["setup-scripts"]` to `.config/nextest.toml` - **Tracking issue:** [#978](https://github.com/nextest-rs/nextest/issues/978) -Nextest supports running _setup scripts_ before tests are run. Setup scripts can be scoped to -particular tests via [filtersets](../filtersets/index.md). +!!! experimental "Experimental: Pre-timeout scripts are not yet stable" -Setup scripts are configured in two parts: _defining scripts_, and _setting up rules_ for when they should be executed. + - **Enable with:** Add `experimental = ["pre-timeout-scripts"]` to `.config/nextest.toml` + - **Tracking issue:** [#978](https://github.com/nextest-rs/nextest/issues/978) + +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_, which are executed at the start of a test run. + * _Pre-timeout scripts_, which are executed before nextest begins terminating a test that has exceeded its timeout. + +Scripts are configured in two parts: _defining scripts_, and _setting up rules_ for when they should be executed. ## Defining scripts -Setup scripts are defined using the top-level `script` configuration. For example, to define a script named "my-script", which runs `my-script.sh`: +Scripts are defined using the top-level `script` configuration. For example, to define a script named "my-script", which runs `my-script.sh`: -```toml title="Setup script definition in .config/nextest.toml" +```toml title="Script definition in .config/nextest.toml" [script.my-script] command = 'my-script.sh' ``` @@ -36,16 +45,16 @@ command = 'script.sh -c "Hello, world!"' command = ['script.sh', '-c', 'Hello, world!'] ``` -Setup scripts can have the following configuration options attached to them: +Scripts can have the following configuration options attached to them: -- **`slow-timeout`**: Mark a setup 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, setup scripts are not marked as slow or terminated (this is different from the slow timeout for tests). -- **`leak-timeout`**: Mark setup scripts [leaky](../features/leaky-tests.md) after a timeout, using the same configuration as for tests. By default, the leak timeout is 100ms. +- **`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. - **`capture-stdout`**: `true` if the script's standard output should be captured, `false` if not. By default, this is `false`. - **`capture-stderr`**: `true` if the script's standard error should be captured, `false` if not. By default, this is `false`. ### Example -```toml title="Advanced setup script definition" +```toml title="Advanced script definition" [script.db-generate] command = 'cargo run -p db-generate' slow-timeout = { period = "60s", terminate-after = 2 } @@ -56,7 +65,7 @@ capture-stderr = false ## Setting up rules -In configuration, you can create rules for when to use scripts on a per-profile basis. This is done via the `profile..scripts` array. For example, you can set up a script that generates a database if tests from the `db-tests` package, or any packages that depend on it, are run. +In configuration, you can create rules for when to use scripts on a per-profile basis. This is done via the `profile..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]] @@ -66,7 +75,7 @@ setup = 'db-generate' (This example uses the `rdeps` [filterset](../filtersets/index.md) predicate.) -Setup scripts can also filter based on platform, using the rules listed in [_Specifying platforms_](../configuration/specifying-platforms.md): +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]] @@ -82,13 +91,32 @@ 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'] +``` + ## Script execution +### Setup scripts + A given setup script _S_ is only executed if the current profile has at least one rule where the `filter` and `platform` predicates match the current execution environment, and the setup script _S_ is listed in `setup`. Setup scripts are executed serially, in the order they are defined (_not_ the order they're specified in the rules). If any setup script exits with a non-zero exit code, the entire test run is terminated. -### Environment variables +#### Environment variables Setup scripts can define environment variables that will be exposed to tests that match the script. This is done by writing to the `$NEXTEST_ENV` environment variable from within the script. @@ -126,6 +154,18 @@ fn my_env_test() { } ``` +### Pre-timeout scripts + +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 sets the following environment variables when executing a pre-timeout script: + + * **`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`**: the name of the binary running the test. + ## Setup scripts in JUnit output From 0e7f441dba05f71c9fe98d52f4adc7629695c1c2 Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Wed, 1 Jan 2025 10:42:28 -0500 Subject: [PATCH 02/13] [squashme] split out separate docs for scripts, address feedback --- site/mkdocs.yml | 5 +- site/src/docs/scripts/index.md | 112 ++++++++++++++++++ site/src/docs/scripts/pre-timeout.md | 56 +++++++++ .../scripts.md => scripts/setup.md} | 101 +++------------- 4 files changed, 189 insertions(+), 85 deletions(-) create mode 100644 site/src/docs/scripts/index.md create mode 100644 site/src/docs/scripts/pre-timeout.md rename site/src/docs/{configuration/scripts.md => scripts/setup.md} (51%) diff --git a/site/mkdocs.yml b/site/mkdocs.yml index f66c61cf0ff..82fe94df37b 100644 --- a/site/mkdocs.yml +++ b/site/mkdocs.yml @@ -112,7 +112,10 @@ nav: - "Mutual exclusion and rate-limiting": docs/configuration/test-groups.md - "Environment variables": docs/configuration/env-vars.md - "Extra arguments": docs/configuration/extra-args.md - - docs/configuration/setup-scripts.md + - "Scripts": + - "Overview": docs/scripts/index.md + - docs/scripts/setup.md + - docs/scripts/pre-timeout.md - Machine-readable output: - "About output formats": docs/machine-readable/index.md - "JUnit support": docs/machine-readable/junit.md diff --git a/site/src/docs/scripts/index.md b/site/src/docs/scripts/index.md new file mode 100644 index 00000000000..21bdc88d5fe --- /dev/null +++ b/site/src/docs/scripts/index.md @@ -0,0 +1,112 @@ +--- +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.` configuration. + +For example, to define a setup script named "my-script", which runs `my-script.sh`: + +```toml title="Setup script definition in .config/nextest.toml" +[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 .config/nextest.toml" +[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..script1] +command = 'script.sh -c "Hello, world!"' + +[script..script2] +command = ['script.sh', '-c', 'Hello, world!'] +``` + +### 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 .config/nextest.toml" +[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..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'] +``` diff --git a/site/src/docs/scripts/pre-timeout.md b/site/src/docs/scripts/pre-timeout.md new file mode 100644 index 00000000000..5923e7de98f --- /dev/null +++ b/site/src/docs/scripts/pre-timeout.md @@ -0,0 +1,56 @@ +--- +icon: material/timer-sand-empty +status: experimental +--- + + + +# Pre-timeout scripts + +!!! experimental "Experimental: This feature is not yet stable" + + - **Enable with:** Add `experimental = ["setup-scripts"]` to `.config/nextest.toml` + - **Tracking issue:** [#978](https://github.com/nextest-rs/nextest/issues/978) + + +Nextest runs *pre-timeout scripts* before terminating a test that has exceeded +its timeout. + +## 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 .config/nextest.toml" +[script.pre-timeout.my-script] +command = 'my-script.sh' +``` + +Pre-timeout scripts can have the following configuration options attached to them: + +- TODO + +### Example + +```toml title="Advanced pre-timeout script definition" +[script.pre-timeout.gdb-dump] +command = 'gdb ... TODO' +# 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 sets the following environment variables when executing a pre-timeout script: + + * **`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_PACKAGE_NAME`**: the name of the package in which the test is located. + * **`NEXTEST_PRE_TIMEOUT_TEST_BINARY_NAME`**: the name of the test binary, if known. + * **`NEXTEST_PRE_TIMEOUT_TEST_BINARY_KIND`**: the kind of the test binary, if known. diff --git a/site/src/docs/configuration/scripts.md b/site/src/docs/scripts/setup.md similarity index 51% rename from site/src/docs/configuration/scripts.md rename to site/src/docs/scripts/setup.md index 822d1d51d11..dda34631fac 100644 --- a/site/src/docs/configuration/scripts.md +++ b/site/src/docs/scripts/setup.md @@ -3,59 +3,47 @@ icon: material/ray-start-arrow status: experimental --- -# Scripts +# Setup scripts -!!! experimental "Experimental: Setup scripts are not yet stable" +!!! experimental "Experimental: This feature is not yet stable" - **Enable with:** Add `experimental = ["setup-scripts"]` to `.config/nextest.toml` - **Tracking issue:** [#978](https://github.com/nextest-rs/nextest/issues/978) -!!! experimental "Experimental: Pre-timeout scripts are not yet stable" +Nextest runs *setup scripts* before tests are run. - - **Enable with:** Add `experimental = ["pre-timeout-scripts"]` to `.config/nextest.toml` - - **Tracking issue:** [#978](https://github.com/nextest-rs/nextest/issues/978) - -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_, which are executed at the start of a test run. - * _Pre-timeout scripts_, which are executed before nextest begins terminating a test that has exceeded its timeout. - -Scripts are configured in two parts: _defining scripts_, and _setting up rules_ for when they should be executed. +## Defining setup scripts -## Defining scripts - -Scripts are defined using the top-level `script` configuration. For example, to define a script named "my-script", which runs `my-script.sh`: +Setup scripts are defined using the top-level `script.setup` configuration. For example, to define a script named "my-script", which runs `my-script.sh`: ```toml title="Script definition in .config/nextest.toml" -[script.my-script] +[script.setup.my-script] command = 'my-script.sh' ``` 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.script1] +[script.setup.script1] command = 'script.sh -c "Hello, world!"' -[script.script2] +[script.setup.script2] command = ['script.sh', '-c', 'Hello, world!'] ``` -Scripts can have the following configuration options attached to them: +Setup scripts can have the following configuration options attached to them: -- **`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. +- **`slow-timeout`**: Mark a setup 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 setup scripts [leaky](../features/leaky-tests.md) after a timeout, using the same configuration as for tests. By default, the leak timeout is 100ms. - **`capture-stdout`**: `true` if the script's standard output should be captured, `false` if not. By default, this is `false`. - **`capture-stderr`**: `true` if the script's standard error should be captured, `false` if not. By default, this is `false`. ### Example -```toml title="Advanced script definition" -[script.db-generate] +```toml title="Advanced setup script definition" +[script.setup.db-generate] command = 'cargo run -p db-generate' slow-timeout = { period = "60s", terminate-after = 2 } leak-timeout = "1s" @@ -63,60 +51,17 @@ capture-stdout = true capture-stderr = false ``` -## Setting up rules +## Specifying setup script rules -In configuration, you can create rules for when to use scripts on a per-profile basis. This is done via the `profile..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. +See [_Specifying rules_](index.md#specifying-rules). -```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'] -``` - -## Script execution - -### Setup scripts +## Setup script execution A given setup script _S_ is only executed if the current profile has at least one rule where the `filter` and `platform` predicates match the current execution environment, and the setup script _S_ is listed in `setup`. Setup scripts are executed serially, in the order they are defined (_not_ the order they're specified in the rules). If any setup script exits with a non-zero exit code, the entire test run is terminated. -#### Environment variables +### Environment variables Setup scripts can define environment variables that will be exposed to tests that match the script. This is done by writing to the `$NEXTEST_ENV` environment variable from within the script. @@ -154,18 +99,6 @@ fn my_env_test() { } ``` -### Pre-timeout scripts - -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 sets the following environment variables when executing a pre-timeout script: - - * **`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`**: the name of the binary running the test. - ## Setup scripts in JUnit output From aacf0198fe1ad0613cd527c070245cf392f95069 Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Wed, 1 Jan 2025 12:11:06 -0500 Subject: [PATCH 03/13] [squashme] specify more of the script lifecycle --- site/src/docs/scripts/index.md | 15 +++++++++++++++ site/src/docs/scripts/pre-timeout.md | 27 ++++++++++++++++++++------- site/src/docs/scripts/setup.md | 16 ++-------------- 3 files changed, 37 insertions(+), 21 deletions(-) diff --git a/site/src/docs/scripts/index.md b/site/src/docs/scripts/index.md index 21bdc88d5fe..a94a37f4e06 100644 --- a/site/src/docs/scripts/index.md +++ b/site/src/docs/scripts/index.md @@ -51,6 +51,21 @@ command = 'script.sh -c "Hello, world!"' 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..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. diff --git a/site/src/docs/scripts/pre-timeout.md b/site/src/docs/scripts/pre-timeout.md index 5923e7de98f..938afc7345c 100644 --- a/site/src/docs/scripts/pre-timeout.md +++ b/site/src/docs/scripts/pre-timeout.md @@ -16,6 +16,8 @@ status: experimental 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`: @@ -25,15 +27,19 @@ Pre-timeout scripts are defined using the top-level `script.pre-timeout` configu command = 'my-script.sh' ``` -Pre-timeout scripts can have the following configuration options attached to them: +See [_Defining scripts_](index.md#defining-scripts) for options that are common to all scripts. + +Pre-timeout scripts do not support additional configuration options. -- TODO +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 = 'gdb ... TODO' +command = ['sh', '-c', 'gdb -p $NEXTEST_PRE_TIMEOUT_TEST_PID -batch -ex "thread apply all backtrace"'] # TODO options ``` @@ -47,10 +53,17 @@ A given pre-timeout script _S_ is executed when the current profile has at least 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 sets the following environment variables when executing a pre-timeout script: +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. + +Nextest executes pre-timeout scripts with the same working directory as the test and sets the following variables in the script's environment: * **`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_PACKAGE_NAME`**: the name of the package in which the test is located. - * **`NEXTEST_PRE_TIMEOUT_TEST_BINARY_NAME`**: the name of the test binary, if known. - * **`NEXTEST_PRE_TIMEOUT_TEST_BINARY_KIND`**: the kind of the test binary, if known. + * **`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. + + diff --git a/site/src/docs/scripts/setup.md b/site/src/docs/scripts/setup.md index dda34631fac..5d2cf88e224 100644 --- a/site/src/docs/scripts/setup.md +++ b/site/src/docs/scripts/setup.md @@ -23,20 +23,10 @@ Setup scripts are defined using the top-level `script.setup` configuration. For command = 'my-script.sh' ``` -Commands can either be specified using Unix shell rules, or as a list of arguments. In the following example, `script1` and `script2` are equivalent. +See [_Defining scripts_](index.md#defining-scripts) for options that are common to all scripts. -```toml -[script.setup.script1] -command = 'script.sh -c "Hello, world!"' - -[script.setup.script2] -command = ['script.sh', '-c', 'Hello, world!'] -``` - -Setup scripts can have the following configuration options attached to them: +Setup scripts support the following additional configuration options: -- **`slow-timeout`**: Mark a setup 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 setup scripts [leaky](../features/leaky-tests.md) after a timeout, using the same configuration as for tests. By default, the leak timeout is 100ms. - **`capture-stdout`**: `true` if the script's standard output should be captured, `false` if not. By default, this is `false`. - **`capture-stderr`**: `true` if the script's standard error should be captured, `false` if not. By default, this is `false`. @@ -45,8 +35,6 @@ Setup scripts can have the following configuration options attached to them: ```toml title="Advanced setup script definition" [script.setup.db-generate] command = 'cargo run -p db-generate' -slow-timeout = { period = "60s", terminate-after = 2 } -leak-timeout = "1s" capture-stdout = true capture-stderr = false ``` From a107acf174d4da3f3d761074d919064ccc0267fe Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Thu, 2 Jan 2025 07:00:17 +0300 Subject: [PATCH 04/13] [squashme] rendering fixes --- site/src/docs/scripts/index.md | 2 -- site/src/docs/scripts/pre-timeout.md | 18 +++++++++--------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/site/src/docs/scripts/index.md b/site/src/docs/scripts/index.md index a94a37f4e06..59aae9a8382 100644 --- a/site/src/docs/scripts/index.md +++ b/site/src/docs/scripts/index.md @@ -4,8 +4,6 @@ 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: diff --git a/site/src/docs/scripts/pre-timeout.md b/site/src/docs/scripts/pre-timeout.md index 938afc7345c..bf0043145d3 100644 --- a/site/src/docs/scripts/pre-timeout.md +++ b/site/src/docs/scripts/pre-timeout.md @@ -3,14 +3,14 @@ icon: material/timer-sand-empty status: experimental --- - + # Pre-timeout scripts !!! experimental "Experimental: This feature is not yet stable" - - **Enable with:** Add `experimental = ["setup-scripts"]` to `.config/nextest.toml` - - **Tracking issue:** [#978](https://github.com/nextest-rs/nextest/issues/978) + - **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 @@ -59,11 +59,11 @@ The pre-timeout script is not responsible for terminating the test process, but Nextest executes pre-timeout scripts with the same working directory as the test and sets the following variables in the script's environment: - * **`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. +* **`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. From 4e5a0877f02367c86a2c7df39f0469885ce4972b Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Sun, 5 Jan 2025 14:48:56 +0300 Subject: [PATCH 05/13] [squashme] fix link to setup scripts --- site/src/docs/configuration/env-vars.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/docs/configuration/env-vars.md b/site/src/docs/configuration/env-vars.md index 52d6d807ffb..da7e2489342 100644 --- a/site/src/docs/configuration/env-vars.md +++ b/site/src/docs/configuration/env-vars.md @@ -165,6 +165,6 @@ Nextest sets the dynamic library path at runtime, similar to [what Cargo does](h Nextest includes the following paths: -- Search paths included from any build script with the [`rustc-link-search` instruction](https://doc.rust-lang.org/cargo/reference/build-scripts.html#rustc-link-search). Paths outside of the target directory are removed. If additional libraries on the system are needed in the search path, consider using a [setup script ](setup-scripts.md) to configure the environment. +- Search paths included from any build script with the [`rustc-link-search` instruction](https://doc.rust-lang.org/cargo/reference/build-scripts.html#rustc-link-search). Paths outside of the target directory are removed. If additional libraries on the system are needed in the search path, consider using a [setup script ](../scripts/setup.md) to configure the environment. - The base output directory, such as `target/debug`, and the "deps" directory. This enables support for `dylib` dependencies and rustc compiler plugins. - The rustc sysroot library path, to enable proc-macro tests and binaries compiled with `-C prefer-dynamic` to work. From 6fa63e62856ca5ef32135882e6d120eeac525990 Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Sun, 5 Jan 2025 14:49:19 +0300 Subject: [PATCH 06/13] [squashme] start plumbing setup scripts --- nextest-runner/src/config/config_impl.rs | 43 ++++++++++++++++---- nextest-runner/src/config/nextest_version.rs | 6 ++- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/nextest-runner/src/config/config_impl.rs b/nextest-runner/src/config/config_impl.rs index fbd7cd7f645..61f5a3321a4 100644 --- a/nextest-runner/src/config/config_impl.rs +++ b/nextest-runner/src/config/config_impl.rs @@ -328,8 +328,8 @@ impl NextestConfig { known_groups.extend(valid_groups); - // If scripts are present, check that the experimental feature is enabled. - if !this_config.scripts.is_empty() + // If setup scripts are present, check that the experimental feature is enabled. + if !this_config.scripts.setup.is_empty() && !experimental.contains(&ConfigExperimental::SetupScripts) { return Err(ConfigParseError::new( @@ -341,9 +341,22 @@ impl NextestConfig { )); } + // If pre-timeout scripts are present, check that the experimental feature is enabled. + if !this_config.scripts.pre_timeout.is_empty() + && !experimental.contains(&ConfigExperimental::PreTimeoutScripts) + { + return Err(ConfigParseError::new( + config_file, + tool, + ConfigParseErrorKind::ExperimentalFeatureNotEnabled { + feature: ConfigExperimental::PreTimeoutScripts, + }, + )); + } + // Check that setup scripts are named as expected. let (valid_scripts, invalid_scripts): (BTreeSet<_>, _) = - this_config.scripts.keys().cloned().partition(|script| { + this_config.scripts.setup.keys().cloned().partition(|script| { if let Some(tool) = tool { // The first component must be the tool name. script @@ -356,6 +369,10 @@ impl NextestConfig { } }); + // TODO: validate pre-timeout scripts. + + // TODO: validate no overlap in names between setup and pre-timeout scripts. + if !invalid_scripts.is_empty() { let kind = if tool.is_some() { ConfigParseErrorKind::InvalidConfigScriptsDefinedByTool(invalid_scripts) @@ -578,8 +595,7 @@ pub struct EarlyProfile<'cfg> { default_profile: &'cfg DefaultProfileImpl, custom_profile: Option<&'cfg CustomProfileImpl>, test_groups: &'cfg BTreeMap, - // This is ordered because the scripts are used in the order they're defined. - scripts: &'cfg IndexMap, + scripts: &'cfg Scripts, // Invariant: `compiled_data.default_filter` is always present. pub(super) compiled_data: CompiledData, } @@ -646,7 +662,7 @@ pub struct EvaluatableProfile<'cfg> { custom_profile: Option<&'cfg CustomProfileImpl>, test_groups: &'cfg BTreeMap, // This is ordered because the scripts are used in the order they're defined. - scripts: &'cfg IndexMap, + scripts: &'cfg Scripts, // Invariant: `compiled_data.default_filter` is always present. pub(super) compiled_data: CompiledData, // The default filter that's been resolved after considering overrides (i.e. @@ -684,7 +700,7 @@ impl<'cfg> EvaluatableProfile<'cfg> { /// Returns the global script configuration. pub fn script_config(&self) -> &'cfg IndexMap { - self.scripts + &self.scripts.setup } /// Returns the retry count for this profile. @@ -809,7 +825,7 @@ impl<'cfg> EvaluatableProfile<'cfg> { pub(super) struct NextestConfigImpl { store: StoreConfigImpl, test_groups: BTreeMap, - scripts: IndexMap, + scripts: Scripts, default_profile: DefaultProfileImpl, other_profiles: HashMap, } @@ -863,7 +879,7 @@ struct NextestConfigDeserialize { #[serde(default)] test_groups: BTreeMap, #[serde(default, rename = "script")] - scripts: IndexMap, + scripts: Scripts, #[serde(rename = "profile")] profiles: HashMap, } @@ -1024,6 +1040,15 @@ impl CustomProfileImpl { } } +#[derive(Clone, Debug, Default, Deserialize)] +#[serde(rename_all = "kebab-case")] +struct Scripts { + // This is ordered because setup scripts are used in the order they're defined. + setup: IndexMap, + // This is not ordered because pre-timeout scripts follow different precedence rules. + pre_timeout: HashMap, +} + #[cfg(test)] mod tests { use super::*; diff --git a/nextest-runner/src/config/nextest_version.rs b/nextest-runner/src/config/nextest_version.rs index 84dc3b0b346..fcac7c71dff 100644 --- a/nextest-runner/src/config/nextest_version.rs +++ b/nextest-runner/src/config/nextest_version.rs @@ -232,11 +232,13 @@ impl NextestVersionConfig { pub enum ConfigExperimental { /// Enable support for setup scripts. SetupScripts, + /// Enable support for pre-timeout scripts. + PreTimeoutScripts, } impl ConfigExperimental { fn known() -> impl Iterator { - vec![Self::SetupScripts].into_iter() + vec![Self::SetupScripts, Self::PreTimeoutScripts].into_iter() } } @@ -246,6 +248,7 @@ impl FromStr for ConfigExperimental { fn from_str(s: &str) -> Result { match s { "setup-scripts" => Ok(Self::SetupScripts), + "pre-timeout-scripts" => Ok(Self::PreTimeoutScripts), _ => Err(()), } } @@ -255,6 +258,7 @@ impl fmt::Display for ConfigExperimental { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::SetupScripts => write!(f, "setup-scripts"), + Self::PreTimeoutScripts => write!(f, "pre-timeout-scripts"), } } } From b2bc33772a7bbd5fdd6ed7c6a78b8584e5344edc Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Sun, 5 Jan 2025 22:06:56 +0300 Subject: [PATCH 07/13] [squashme] sequentialize execution --- site/src/docs/scripts/pre-timeout.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/site/src/docs/scripts/pre-timeout.md b/site/src/docs/scripts/pre-timeout.md index bf0043145d3..d72c8190226 100644 --- a/site/src/docs/scripts/pre-timeout.md +++ b/site/src/docs/scripts/pre-timeout.md @@ -49,9 +49,11 @@ 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. +A given pre-timeout script _S_ is a candidate for execution 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` _T_ 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. +Only the _first_ candidate pre-timeout script will be executed for each such _T_. Nextest orders execution candidates by their definition order (_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. From a9103e6f425072469606981c95c74932d64dbee8 Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Sun, 5 Jan 2025 22:07:12 +0300 Subject: [PATCH 08/13] [squashme] parse and validate pre-timeout scripts --- nextest-runner/src/config/config_impl.rs | 132 +++++++++++++++++------ nextest-runner/src/config/overrides.rs | 2 +- nextest-runner/src/config/scripts.rs | 21 ++++ nextest-runner/src/errors.rs | 17 ++- 4 files changed, 136 insertions(+), 36 deletions(-) diff --git a/nextest-runner/src/config/config_impl.rs b/nextest-runner/src/config/config_impl.rs index 61f5a3321a4..39d53d3f9db 100644 --- a/nextest-runner/src/config/config_impl.rs +++ b/nextest-runner/src/config/config_impl.rs @@ -9,6 +9,7 @@ use super::{ TestThreads, ThreadsRequired, ToolConfigFile, }; use crate::{ + config::ScriptType, errors::{ provided_by_tool, ConfigParseError, ConfigParseErrorKind, ProfileNotFound, UnknownConfigScriptError, UnknownTestGroupError, @@ -354,9 +355,20 @@ impl NextestConfig { )); } - // Check that setup scripts are named as expected. - let (valid_scripts, invalid_scripts): (BTreeSet<_>, _) = - this_config.scripts.setup.keys().cloned().partition(|script| { + if let Some(dup) = this_config.scripts.duplicate_ids().next() { + return Err(ConfigParseError::new( + config_file, + tool, + ConfigParseErrorKind::DuplicateConfigScriptName(dup.clone()), + )); + } + + // Check that scripts are named as expected. + let (valid_scripts, invalid_scripts): (BTreeSet<_>, _) = this_config + .scripts + .all_script_ids() + .cloned() + .partition(|script| { if let Some(tool) = tool { // The first component must be the tool name. script @@ -369,10 +381,6 @@ impl NextestConfig { } }); - // TODO: validate pre-timeout scripts. - - // TODO: validate no overlap in names between setup and pre-timeout scripts. - if !invalid_scripts.is_empty() { let kind = if tool.is_some() { ConfigParseErrorKind::InvalidConfigScriptsDefinedByTool(invalid_scripts) @@ -451,40 +459,69 @@ impl NextestConfig { // Check that scripts are known. let mut unknown_script_errors = Vec::new(); - let mut check_script_ids = |profile_name: &str, scripts: &[ScriptId]| { - if !scripts.is_empty() && !experimental.contains(&ConfigExperimental::SetupScripts) { - return Err(ConfigParseError::new( - config_file, - tool, - ConfigParseErrorKind::ExperimentalFeatureNotEnabled { - feature: ConfigExperimental::SetupScripts, - }, - )); - } - for script in scripts { - if !known_scripts.contains(script) { - unknown_script_errors.push(UnknownConfigScriptError { - profile_name: profile_name.to_owned(), - name: script.clone(), - }); + let mut check_script_ids = + |profile_name: &str, script_type: ScriptType, scripts: &[ScriptId]| { + let config_feature = match script_type { + ScriptType::Setup => ConfigExperimental::SetupScripts, + ScriptType::PreTimeout => ConfigExperimental::PreTimeoutScripts, + }; + if !scripts.is_empty() && !experimental.contains(&config_feature) { + return Err(ConfigParseError::new( + config_file, + tool, + ConfigParseErrorKind::ExperimentalFeatureNotEnabled { + feature: config_feature, + }, + )); + } + for script in scripts { + if !known_scripts.contains(script) { + unknown_script_errors.push(UnknownConfigScriptError { + profile_name: profile_name.to_owned(), + name: script.clone(), + }); + } + let actual_script_type = this_config.scripts.script_type(script); + if actual_script_type != script_type { + return Err(ConfigParseError::new( + config_file, + tool, + ConfigParseErrorKind::WrongConfigScriptType { + script: script.clone(), + attempted: script_type, + actual: actual_script_type, + }, + )); + } } - } - Ok(()) - }; + Ok(()) + }; this_compiled .default .scripts .iter() - .try_for_each(|scripts| check_script_ids("default", &scripts.setup))?; + .try_for_each(|scripts| { + check_script_ids("default", ScriptType::Setup, &scripts.setup)?; + check_script_ids( + "default", + ScriptType::PreTimeout, + scripts.pre_timeout.as_slice(), + ) + })?; this_compiled .other .iter() .try_for_each(|(profile_name, data)| { - data.scripts - .iter() - .try_for_each(|scripts| check_script_ids(profile_name, &scripts.setup)) + data.scripts.iter().try_for_each(|scripts| { + check_script_ids(profile_name, ScriptType::Setup, &scripts.setup)?; + check_script_ids( + profile_name, + ScriptType::PreTimeout, + scripts.pre_timeout.as_slice(), + ) + }) })?; // If there were any unknown scripts, error out. @@ -978,7 +1015,7 @@ impl DefaultProfileImpl { &self.overrides } - pub(super) fn setup_scripts(&self) -> &[DeserializedProfileScriptConfig] { + pub(super) fn scripts(&self) -> &[DeserializedProfileScriptConfig] { &self.scripts } } @@ -1043,10 +1080,37 @@ impl CustomProfileImpl { #[derive(Clone, Debug, Default, Deserialize)] #[serde(rename_all = "kebab-case")] struct Scripts { - // This is ordered because setup scripts are used in the order they're defined. + // These maps are ordered because scripts are used in the order they're defined. setup: IndexMap, - // This is not ordered because pre-timeout scripts follow different precedence rules. - pre_timeout: HashMap, + pre_timeout: IndexMap, +} + +impl Scripts { + /// Determines the type of the script with the given ID. + /// + /// Panics if the ID is invalid. + fn script_type(&self, id: &ScriptId) -> ScriptType { + if self.setup.contains_key(id) { + ScriptType::Setup + } else if self.pre_timeout.contains_key(id) { + ScriptType::PreTimeout + } else { + panic!("Scripts::script_type called with invalid script ID: {id}") + } + } + + /// Returns an iterator over the names of all scripts of all types. + fn all_script_ids(&self) -> impl Iterator { + self.setup.keys().chain(self.pre_timeout.keys()) + } + + /// Returns an iterator over names that are used by more than one type of + /// script. + fn duplicate_ids(&self) -> impl Iterator { + self.pre_timeout + .keys() + .filter(|k| self.setup.contains_key(*k)) + } } #[cfg(test)] diff --git a/nextest-runner/src/config/overrides.rs b/nextest-runner/src/config/overrides.rs index d9d021d5b39..cd5f9855009 100644 --- a/nextest-runner/src/config/overrides.rs +++ b/nextest-runner/src/config/overrides.rs @@ -295,7 +295,7 @@ impl CompiledByProfile { "default", Some(config.default_profile().default_filter()), config.default_profile().overrides(), - config.default_profile().setup_scripts(), + config.default_profile().scripts(), &mut errors, ); let other: HashMap<_, _> = config diff --git a/nextest-runner/src/config/scripts.rs b/nextest-runner/src/config/scripts.rs index 5911524592b..905042317ae 100644 --- a/nextest-runner/src/config/scripts.rs +++ b/nextest-runner/src/config/scripts.rs @@ -246,6 +246,7 @@ impl<'profile> SetupScriptExecuteData<'profile> { #[derive(Clone, Debug)] pub(crate) struct CompiledProfileScripts { pub(super) setup: Vec, + pub(super) pre_timeout: Option, pub(super) data: ProfileScriptData, state: State, } @@ -289,6 +290,7 @@ impl CompiledProfileScripts { match (host_spec, target_spec, filter_expr) { (Ok(host_spec), Ok(target_spec), Ok(expr)) => Some(Self { setup: source.setup.clone(), + pre_timeout: source.pre_timeout.clone(), data: ProfileScriptData { host_spec, target_spec, @@ -330,6 +332,7 @@ impl CompiledProfileScripts { CompiledProfileScripts { setup: self.setup, + pre_timeout: self.pre_timeout, data: self.data, state: FinalConfig { host_eval, @@ -399,6 +402,21 @@ impl fmt::Display for ScriptId { } } +#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, PartialOrd, Ord)] +pub enum ScriptType { + Setup, + PreTimeout, +} + +impl fmt::Display for ScriptType { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + ScriptType::Setup => f.write_str("setup"), + ScriptType::PreTimeout => f.write_str("pre-timeout"), + } + } +} + #[derive(Clone, Debug)] pub(super) struct ProfileScriptData { host_spec: MaybeTargetSpec, @@ -421,6 +439,9 @@ pub(super) struct DeserializedProfileScriptConfig { /// The setup script or scripts to run. #[serde(deserialize_with = "deserialize_script_ids")] setup: Vec, + + /// The pre-timeout script to run. + pre_timeout: Option, } /// Deserialized form of script configuration before compilation. diff --git a/nextest-runner/src/errors.rs b/nextest-runner/src/errors.rs index 1ac085e02d3..3d8d17b0c4b 100644 --- a/nextest-runner/src/errors.rs +++ b/nextest-runner/src/errors.rs @@ -5,7 +5,7 @@ use crate::{ cargo_config::{TargetTriple, TargetTripleSource}, - config::{ConfigExperimental, CustomTestGroup, ScriptId, TestGroup}, + config::{ConfigExperimental, CustomTestGroup, ScriptId, ScriptType, TestGroup}, helpers::{display_exited_with, dylib_path_envvar}, redact::Redactor, reuse_build::{ArchiveFormat, ArchiveStep}, @@ -124,6 +124,21 @@ pub enum ConfigParseErrorKind { #[error( "invalid config scripts defined by tool: {}\n(config scripts must start with '@tool::')", .0.iter().join(", "))] InvalidConfigScriptsDefinedByTool(BTreeSet), + /// The same config script name was used across config script types. + #[error("config script name used more than once: {0}\n(config script names must be unique across all script types)")] + DuplicateConfigScriptName(ScriptId), + /// The same config script name was used across config script types. + #[error("cannot use config script as a {attempted} script: {script}\n(config script is a {actual} script)")] + WrongConfigScriptType { + /// The name of the config script. + script: ScriptId, + + /// The script type that the user attempted to use the script as. + attempted: ScriptType, + + /// The actual script type. + actual: ScriptType, + }, /// Some config scripts were unknown. #[error( "unknown config scripts specified by config (destructure this variant for more details)" From 9b68ab586f03e8b80570d206d4d40c71200ab136 Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Wed, 8 Jan 2025 21:17:19 +0000 Subject: [PATCH 09/13] [squashme] first working implementation --- nextest-runner/src/config/config_impl.rs | 26 +- nextest-runner/src/config/overrides.rs | 12 +- nextest-runner/src/config/scripts.rs | 146 +++++++- .../src/reporter/aggregator/junit.rs | 6 + nextest-runner/src/reporter/displayer.rs | 168 +++++++-- nextest-runner/src/reporter/events.rs | 75 ++++ nextest-runner/src/runner/dispatcher.rs | 32 +- nextest-runner/src/runner/executor.rs | 354 +++++++++++++++++- nextest-runner/src/runner/internal_events.rs | 45 ++- nextest-runner/src/runner/unix.rs | 2 +- 10 files changed, 786 insertions(+), 80 deletions(-) diff --git a/nextest-runner/src/config/config_impl.rs b/nextest-runner/src/config/config_impl.rs index 39d53d3f9db..6aa4e430299 100644 --- a/nextest-runner/src/config/config_impl.rs +++ b/nextest-runner/src/config/config_impl.rs @@ -4,17 +4,17 @@ use super::{ ArchiveConfig, CompiledByProfile, CompiledData, CompiledDefaultFilter, ConfigExperimental, CustomTestGroup, DefaultJunitImpl, DeserializedOverride, DeserializedProfileScriptConfig, - JunitConfig, JunitImpl, NextestVersionDeserialize, RetryPolicy, ScriptConfig, ScriptId, - SettingSource, SetupScripts, SlowTimeout, TestGroup, TestGroupConfig, TestSettings, + JunitConfig, JunitImpl, NextestVersionDeserialize, RetryPolicy, ScriptId, SettingSource, + SetupScriptConfig, SetupScripts, SlowTimeout, TestGroup, TestGroupConfig, TestSettings, TestThreads, ThreadsRequired, ToolConfigFile, }; use crate::{ - config::ScriptType, + config::{PreTimeoutScript, PreTimeoutScriptConfig, ScriptType}, errors::{ provided_by_tool, ConfigParseError, ConfigParseErrorKind, ProfileNotFound, UnknownConfigScriptError, UnknownTestGroupError, }, - list::TestList, + list::{TestInstance, TestList}, platform::BuildPlatforms, reporter::{FinalStatusLevel, StatusLevel, TestOutputDisplay}, }; @@ -736,8 +736,8 @@ impl<'cfg> EvaluatableProfile<'cfg> { } /// Returns the global script configuration. - pub fn script_config(&self) -> &'cfg IndexMap { - &self.scripts.setup + pub fn script_config(&self) -> &'cfg Scripts { + &self.scripts } /// Returns the retry count for this profile. @@ -830,6 +830,11 @@ impl<'cfg> EvaluatableProfile<'cfg> { SetupScripts::new(self, test_list) } + /// Returns the pre-timeout script for the specified test, if it exists. + pub fn pre_timeout_script(&self, test_instance: &TestInstance<'_>) -> Option { + PreTimeoutScript::new(self, test_instance) + } + /// Returns settings for individual tests. pub fn settings_for(&self, query: &TestQuery<'_>) -> TestSettings { TestSettings::new(self, query) @@ -1077,12 +1082,15 @@ impl CustomProfileImpl { } } +/// The scripts defined in a profile. #[derive(Clone, Debug, Default, Deserialize)] #[serde(rename_all = "kebab-case")] -struct Scripts { +pub struct Scripts { // These maps are ordered because scripts are used in the order they're defined. - setup: IndexMap, - pre_timeout: IndexMap, + /// The setup scripts defined in a profile. + pub setup: IndexMap, + /// The pre-timeout scripts defined in a profile. + pub pre_timeout: IndexMap, } impl Scripts { diff --git a/nextest-runner/src/config/overrides.rs b/nextest-runner/src/config/overrides.rs index cd5f9855009..a79aac63d0a 100644 --- a/nextest-runner/src/config/overrides.rs +++ b/nextest-runner/src/config/overrides.rs @@ -486,13 +486,13 @@ impl CompiledData { pub(super) fn chain(self, other: Self) -> Self { let profile_default_filter = self.profile_default_filter.or(other.profile_default_filter); let mut overrides = self.overrides; - let mut setup_scripts = self.scripts; + let mut scripts = self.scripts; overrides.extend(other.overrides); - setup_scripts.extend(other.scripts); + scripts.extend(other.scripts); Self { profile_default_filter, overrides, - scripts: setup_scripts, + scripts, } } @@ -506,15 +506,15 @@ impl CompiledData { .into_iter() .map(|override_| override_.apply_build_platforms(build_platforms)) .collect(); - let setup_scripts = self + let scripts = self .scripts .into_iter() - .map(|setup_script| setup_script.apply_build_platforms(build_platforms)) + .map(|script| script.apply_build_platforms(build_platforms)) .collect(); CompiledData { profile_default_filter, overrides, - scripts: setup_scripts, + scripts, } } } diff --git a/nextest-runner/src/config/scripts.rs b/nextest-runner/src/config/scripts.rs index 905042317ae..af0a196b39e 100644 --- a/nextest-runner/src/config/scripts.rs +++ b/nextest-runner/src/config/scripts.rs @@ -13,7 +13,7 @@ use crate::{ ChildStartError, ConfigCompileError, ConfigCompileErrorKind, ConfigCompileSection, InvalidConfigScriptName, }, - list::TestList, + list::{TestInstance, TestList}, platform::BuildPlatforms, reporter::events::SetupScriptEnvMap, test_command::{apply_ld_dyld_env, create_command}, @@ -91,7 +91,7 @@ impl<'profile> SetupScripts<'profile> { // Build up a map of enabled scripts along with their data, by script ID. let mut enabled_scripts = IndexMap::new(); - for (script_id, config) in script_config { + for (script_id, config) in &script_config.setup { if enabled_ids.contains(script_id) { let compiled = by_script_id .remove(script_id) @@ -139,7 +139,7 @@ pub(crate) struct SetupScript<'profile> { pub(crate) id: ScriptId, /// The configuration for the script. - pub(crate) config: &'profile ScriptConfig, + pub(crate) config: &'profile SetupScriptConfig, /// The compiled filters to use to check which tests this script is enabled for. pub(crate) compiled: Vec<&'profile CompiledProfileScripts>, @@ -166,7 +166,7 @@ pub(crate) struct SetupScriptCommand { impl SetupScriptCommand { /// Creates a new `SetupScriptCommand` for a setup script. pub(crate) fn new( - config: &ScriptConfig, + config: &SetupScriptConfig, double_spawn: &DoubleSpawnInfo, test_list: &TestList<'_>, ) -> Result { @@ -243,6 +243,98 @@ impl<'profile> SetupScriptExecuteData<'profile> { } } +/// Data about a pre-timeout script, returned by an [`EvaluatableProfile`]. +#[derive(Debug)] +pub struct PreTimeoutScript<'profile> { + /// The script ID. + pub id: ScriptId, + + /// The configuration for the script. + pub config: &'profile PreTimeoutScriptConfig, +} + +impl<'profile> PreTimeoutScript<'profile> { + pub(super) fn new( + profile: &'profile EvaluatableProfile<'_>, + test_instance: &TestInstance<'_>, + ) -> Option { + let test_query = test_instance.to_test_query(); + + let profile_scripts = &profile.compiled_data.scripts; + if profile_scripts.is_empty() { + return None; + } + + let env = profile.filterset_ecx(); + for compiled in profile_scripts { + if let Some(script_id) = &compiled.pre_timeout { + if compiled.is_enabled(&test_query, &env) { + let config = &profile.script_config().pre_timeout[script_id]; + return Some(PreTimeoutScript { + id: script_id.clone(), + config, + }); + } + } + } + + None + } +} + +/// Represents a to-be-run pre-timeout script command with a certain set of arguments. +pub(crate) struct PreTimeoutScriptCommand { + /// The command to be run. + command: std::process::Command, + /// Double-spawn context. + double_spawn: Option, +} + +impl PreTimeoutScriptCommand { + /// Creates a new `PreTimeoutScriptCommand` for a setup script. + pub(crate) fn new( + config: &PreTimeoutScriptConfig, + double_spawn: &DoubleSpawnInfo, + test_list: &TestList<'_>, + test_instance: &TestInstance<'_>, + ) -> Result { + let mut cmd = create_command(config.program().to_owned(), config.args(), double_spawn); + + // NB: we will always override user-provided environment variables with the + // `CARGO_*` and `NEXTEST_*` variables set directly on `cmd` below. + test_list.cargo_env().apply_env(&mut cmd); + + cmd.current_dir(&test_instance.suite_info.cwd) + // This environment variable is set to indicate that tests are being run under nextest. + .env("NEXTEST", "1"); + + apply_ld_dyld_env(&mut cmd, test_list.updated_dylib_path()); + + let double_spawn = double_spawn.spawn_context(); + + Ok(Self { + command: cmd, + double_spawn, + }) + } + + /// Returns the command to be run. + #[inline] + pub(crate) fn command_mut(&mut self) -> &mut std::process::Command { + &mut self.command + } + + pub(crate) fn spawn(self) -> std::io::Result { + let mut command = tokio::process::Command::from(self.command); + let res = command.spawn(); + if let Some(ctx) = self.double_spawn { + ctx.finish(); + } + let child = res?; + Ok(child) + } +} + #[derive(Clone, Debug)] pub(crate) struct CompiledProfileScripts { pub(super) setup: Vec, @@ -444,12 +536,12 @@ pub(super) struct DeserializedProfileScriptConfig { pre_timeout: Option, } -/// Deserialized form of script configuration before compilation. +/// Deserialized form of setup script configuration before compilation. /// /// This is defined as a top-level element. #[derive(Clone, Debug, Deserialize)] #[serde(rename_all = "kebab-case")] -pub struct ScriptConfig { +pub struct SetupScriptConfig { /// The command to run. The first element is the program and the second element is a list /// of arguments. #[serde(deserialize_with = "deserialize_command")] @@ -473,10 +565,10 @@ pub struct ScriptConfig { /// JUnit configuration for this script. #[serde(default)] - pub junit: ScriptJunitConfig, + pub junit: SetupScriptJunitConfig, } -impl ScriptConfig { +impl SetupScriptConfig { /// Returns the name of the program. #[inline] pub fn program(&self) -> &str { @@ -499,7 +591,7 @@ impl ScriptConfig { /// A JUnit override configuration. #[derive(Copy, Clone, Debug, Deserialize)] #[serde(rename_all = "kebab-case")] -pub struct ScriptJunitConfig { +pub struct SetupScriptJunitConfig { /// Whether to store successful output. /// /// Defaults to true. @@ -513,7 +605,7 @@ pub struct ScriptJunitConfig { pub store_failure_output: bool, } -impl Default for ScriptJunitConfig { +impl Default for SetupScriptJunitConfig { fn default() -> Self { Self { store_success_output: true, @@ -522,6 +614,40 @@ impl Default for ScriptJunitConfig { } } +/// Deserialized form of pre-timeout script configuration before compilation. +/// +/// This is defined as a top-level element. +#[derive(Clone, Debug, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub struct PreTimeoutScriptConfig { + /// The command to run. The first element is the program and the second element is a list + /// of arguments. + #[serde(deserialize_with = "deserialize_command")] + pub command: (String, Vec), + + /// An optional slow timeout for this command. + #[serde(default, deserialize_with = "super::deserialize_slow_timeout")] + pub slow_timeout: Option, + + /// An optional leak timeout for this command. + #[serde(default, with = "humantime_serde::option")] + pub leak_timeout: Option, +} + +impl PreTimeoutScriptConfig { + /// Returns the name of the program. + #[inline] + pub fn program(&self) -> &str { + &self.command.0 + } + + /// Returns the arguments to the command. + #[inline] + pub fn args(&self) -> &[String] { + &self.command.1 + } +} + fn default_true() -> bool { true } diff --git a/nextest-runner/src/reporter/aggregator/junit.rs b/nextest-runner/src/reporter/aggregator/junit.rs index 1a8e86867ad..f2a1d9d046c 100644 --- a/nextest-runner/src/reporter/aggregator/junit.rs +++ b/nextest-runner/src/reporter/aggregator/junit.rs @@ -100,6 +100,12 @@ impl<'cfg> MetadataJunit<'cfg> { } } } + TestEventKind::PreTimeoutScriptStarted { .. } + | TestEventKind::PreTimeoutScriptSlow { .. } + | TestEventKind::PreTimeoutScriptFinished { .. } => { + // JUnit reports don't have a good place to include information + // about pre-timeout scripts, so we elide them from the report. + } TestEventKind::InfoStarted { .. } | TestEventKind::InfoResponse { .. } | TestEventKind::InfoFinished { .. } => {} diff --git a/nextest-runner/src/reporter/displayer.rs b/nextest-runner/src/reporter/displayer.rs index 9de796277e1..027b78343ef 100644 --- a/nextest-runner/src/reporter/displayer.rs +++ b/nextest-runner/src/reporter/displayer.rs @@ -771,11 +771,74 @@ impl<'a> TestReporterImpl<'a> { // Always display failing setup script output if it exists. We may change this in // the future. if !run_status.result.is_success() { - self.write_setup_script_execute_status( - script_id, command, args, run_status, writer, + self.write_script_execute_status( + script_id, + command, + args, + run_status.result.is_success(), + &run_status.output, + writer, )?; } } + TestEventKind::PreTimeoutScriptStarted { test_instance, .. } => { + writeln!( + writer, + "{:>12} {}", + "PRETMT".style(self.styles.pass), + self.display_test_instance(test_instance.id()), + )?; + } + TestEventKind::PreTimeoutScriptSlow { + test_instance, + elapsed, + will_terminate, + } => { + if !*will_terminate && self.status_levels.status_level >= StatusLevel::Slow { + write!(writer, "{:>12} ", "PRETMT SLOW".style(self.styles.skip))?; + } else if *will_terminate { + write!(writer, "{:>12} ", "PRETMT TERM".style(self.styles.fail))?; + } + + self.write_slow_duration(*elapsed, writer)?; + writeln!(writer, "{}", self.display_test_instance(test_instance.id()))?; + } + TestEventKind::PreTimeoutScriptFinished { + script_id, + command, + args, + test_instance, + run_status, + } => { + match run_status.result { + ExecutionResult::Pass => { + write!(writer, "{:>12} ", "PRETMT PASS".style(self.styles.pass))?; + } + ExecutionResult::Leak => { + write!(writer, "{:>12} ", "PRETMT LEAK".style(self.styles.skip))?; + } + other => { + let status_str = short_status_str(other); + write!( + writer, + "{:>12} ", + format!("PRETMT {status_str}").style(self.styles.fail), + )?; + } + } + + self.write_duration(run_status.time_taken, writer)?; + writeln!(writer, "{}", self.display_test_instance(test_instance.id()))?; + + self.write_script_execute_status( + script_id, + command, + args, + run_status.result.is_success(), + &run_status.output, + writer, + )?; + } TestEventKind::TestStarted { test_instance, .. } => { // In no-capture mode, print out a test start event. if self.no_capture { @@ -1544,35 +1607,39 @@ impl<'a> TestReporterImpl<'a> { writer: &mut dyn Write, ) -> io::Result<()> { let status_str = "status".style(self.styles.count); + + let mut print_running = |pid: u32, time_taken: Duration, slow_after: Option| { + let running_style = if output_has_errors { + self.styles.fail + } else if slow_after.is_some() { + self.styles.skip + } else { + self.styles.pass + }; + write!( + writer, + "{status_str}: {attempt_str}{kind} {} for {:.3?}s as PID {}", + "running".style(running_style), + time_taken.as_secs_f64(), + pid.style(self.styles.count), + )?; + if let Some(slow_after) = slow_after { + write!( + writer, + " (marked slow after {:.3?}s)", + slow_after.as_secs_f64() + )?; + } + writeln!(writer)?; + Ok::<_, io::Error>(()) + }; + match state { UnitState::Running { pid, time_taken, slow_after, - } => { - let running_style = if output_has_errors { - self.styles.fail - } else if slow_after.is_some() { - self.styles.skip - } else { - self.styles.pass - }; - write!( - writer, - "{status_str}: {attempt_str}{kind} {} for {:.3?}s as PID {}", - "running".style(running_style), - time_taken.as_secs_f64(), - pid.style(self.styles.count), - )?; - if let Some(slow_after) = slow_after { - write!( - writer, - " (marked slow after {:.3?}s)", - slow_after.as_secs_f64() - )?; - } - writeln!(writer)?; - } + } => print_running(*pid, *time_taken, *slow_after)?, UnitState::Exiting { pid, time_taken, @@ -1608,6 +1675,42 @@ impl<'a> TestReporterImpl<'a> { )?; } } + UnitState::PreTimeout { + pid, + time_taken, + slow_after, + script_state, + script_output, + } => { + // Print test status. + print_running(*pid, *time_taken, Some(*slow_after))?; + + // Print note about running pre-timeout script. + write!( + writer, + "{}: test has reached timeout, pre-timeout script is running:", + "note".style(self.styles.count) + )?; + + // Print state of the pre-timeout script, indented one level + // further: + let mut writer = IndentWriter::new_skip_initial(" ", writer); + writeln!(&mut writer)?; + self.write_unit_state( + UnitKind::Script, + "", + script_state, + script_output.has_errors(), + &mut writer, + )?; + if script_state.has_valid_output() { + self.write_child_execution_output( + &self.output_spec_for_info(UnitKind::Script), + script_output, + &mut writer, + )?; + } + } UnitState::Terminating(state) => { self.write_terminating_state(kind, attempt_str, state, writer)?; } @@ -1815,16 +1918,17 @@ impl<'a> TestReporterImpl<'a> { write!(writer, "[>{:>7.3?}s] ", duration.as_secs_f64()) } - fn write_setup_script_execute_status( + fn write_script_execute_status( &self, script_id: &ScriptId, command: &str, args: &[String], - run_status: &SetupScriptExecuteStatus, + succeeded: bool, + output: &ChildExecutionOutput, writer: &mut dyn Write, ) -> io::Result<()> { - let spec = self.output_spec_for_script(script_id, command, args, run_status); - self.write_child_execution_output(&spec, &run_status.output, writer) + let spec = self.output_spec_for_script(script_id, command, args, succeeded); + self.write_child_execution_output(&spec, &output, writer) } fn write_test_execute_status( @@ -2109,9 +2213,9 @@ impl<'a> TestReporterImpl<'a> { script_id: &ScriptId, command: &str, args: &[String], - run_status: &SetupScriptExecuteStatus, + succeeded: bool, ) -> ChildOutputSpec { - let header_style = if run_status.result.is_success() { + let header_style = if succeeded { self.styles.pass } else { self.styles.fail diff --git a/nextest-runner/src/reporter/events.rs b/nextest-runner/src/reporter/events.rs index 5a157496cfe..ad0f1945193 100644 --- a/nextest-runner/src/reporter/events.rs +++ b/nextest-runner/src/reporter/events.rs @@ -124,6 +124,42 @@ pub enum TestEventKind<'a> { run_status: SetupScriptExecuteStatus, }, + /// A pre-timeout script started. + PreTimeoutScriptStarted { + /// The test instance that timed out. + test_instance: TestInstance<'a>, + }, + + /// A pre-timeout script was slow. + PreTimeoutScriptSlow { + /// The test instance that timed out. + test_instance: TestInstance<'a>, + + /// The amount of time elapsed since the start of execution. + elapsed: Duration, + + /// True if the script has hit its timeout and is about to be terminated. + will_terminate: bool, + }, + + /// A pre-timeout script completed execution. + PreTimeoutScriptFinished { + /// The script ID. + script_id: ScriptId, + + /// The command to run. + command: &'a str, + + /// The arguments to the command. + args: &'a [String], + + /// The test instance that timed out. + test_instance: TestInstance<'a>, + + /// The execution status of the pre-timeout script. + run_status: PreTimeoutScriptExecuteStatus, + }, + // TODO: add events for BinaryStarted and BinaryFinished? May want a slightly different way to // do things, maybe a couple of reporter traits (one for the run as a whole and one for each // binary). @@ -701,6 +737,25 @@ pub struct SetupScriptEnvMap { pub env_map: BTreeMap, } +/// Information about the execution of a setup script. +#[derive(Clone, Debug)] +pub struct PreTimeoutScriptExecuteStatus { + /// Output for this pre-timeout script. + pub output: ChildExecutionOutput, + + /// The execution result for this setup script: pass, fail or execution error. + pub result: ExecutionResult, + + /// The time at which the script started. + pub start_time: DateTime, + + /// The time it took for the script to run. + pub time_taken: Duration, + + /// Whether this script counts as slow. + pub is_slow: bool, +} + /// Data related to retries for a test. #[derive(Clone, Copy, Debug, Eq, PartialEq, PartialOrd, Ord)] pub struct RetryData { @@ -961,6 +1016,25 @@ pub enum UnitState { remaining: Duration, }, + /// The test has exceeded its timeout and nextest is executing the test's + /// pre-timeout script. (Only relevant for tests.) + PreTimeout { + /// The process ID of the test. + pid: u32, + + /// The amount of time the unit has been running. + time_taken: Duration, + + /// The duration after which the test was marked as slow. + slow_after: Duration, + + /// The state of the pre-timeout script. + script_state: Box, + + /// The output of the pre-timeout script. + script_output: ChildExecutionOutput, + }, + /// The child process is being terminated by nextest. Terminating(UnitTerminatingState), @@ -1000,6 +1074,7 @@ impl UnitState { match self { UnitState::Running { .. } | UnitState::Exiting { .. } + | UnitState::PreTimeout { .. } | UnitState::Terminating(_) | UnitState::Exited { .. } => true, UnitState::DelayBeforeNextAttempt { .. } => false, diff --git a/nextest-runner/src/runner/dispatcher.rs b/nextest-runner/src/runner/dispatcher.rs index 47bbba14782..bf3d3bc50f8 100644 --- a/nextest-runner/src/runner/dispatcher.rs +++ b/nextest-runner/src/runner/dispatcher.rs @@ -9,7 +9,7 @@ use super::{RunUnitRequest, RunnerTaskState, ShutdownRequest}; use crate::{ - config::{MaxFail, ScriptConfig, ScriptId}, + config::{MaxFail, ScriptId, SetupScriptConfig}, input::{InputEvent, InputHandler}, list::{TestInstance, TestInstanceId, TestList}, reporter::events::{ @@ -406,6 +406,32 @@ where HandleEventResponse::None } } + InternalEvent::Executor(ExecutorEvent::PreTimeoutScriptStarted { test_instance }) => { + self.callback_none_response(TestEventKind::PreTimeoutScriptStarted { + test_instance, + }) + } + InternalEvent::Executor(ExecutorEvent::PreTimeoutScriptSlow { + test_instance, + elapsed, + will_terminate, + }) => self.callback_none_response(TestEventKind::PreTimeoutScriptSlow { + test_instance, + elapsed, + will_terminate: will_terminate.is_some(), + }), + InternalEvent::Executor(ExecutorEvent::PreTimeoutScriptFinished { + test_instance, + script_id, + config, + status, + }) => self.callback_none_response(TestEventKind::PreTimeoutScriptFinished { + test_instance, + script_id, + command: config.program(), + args: config.args(), + run_status: status, + }), InternalEvent::Executor(ExecutorEvent::Started { test_instance, req_rx_tx, @@ -540,7 +566,7 @@ where fn new_setup_script( &mut self, id: ScriptId, - config: &'a ScriptConfig, + config: &'a SetupScriptConfig, index: usize, total: usize, req_tx: UnboundedSender>, @@ -796,7 +822,7 @@ struct ContextSetupScript<'a> { id: ScriptId, // Store these details primarily for debugging. #[expect(dead_code)] - config: &'a ScriptConfig, + config: &'a SetupScriptConfig, #[expect(dead_code)] index: usize, #[expect(dead_code)] diff --git a/nextest-runner/src/runner/executor.rs b/nextest-runner/src/runner/executor.rs index 0e91301c720..16ceecfb32c 100644 --- a/nextest-runner/src/runner/executor.rs +++ b/nextest-runner/src/runner/executor.rs @@ -14,8 +14,8 @@ use super::HandleSignalResult; use crate::{ config::{ - EvaluatableProfile, RetryPolicy, ScriptConfig, ScriptId, SetupScriptCommand, - SetupScriptExecuteData, SlowTimeout, TestSettings, + EvaluatableProfile, PreTimeoutScriptCommand, PreTimeoutScriptConfig, RetryPolicy, ScriptId, + SetupScriptCommand, SetupScriptConfig, SetupScriptExecuteData, SlowTimeout, TestSettings, }, double_spawn::DoubleSpawnInfo, errors::{ChildError, ChildFdError, ChildStartError, ErrorList}, @@ -25,7 +25,8 @@ use crate::{ TestInfoResponse, UnitKind, UnitState, }, runner::{ - parse_env_file, ExecutorEvent, InternalExecuteStatus, InternalSetupScriptExecuteStatus, + parse_env_file, ExecutorEvent, InternalExecuteStatus, + InternalPreTimeoutScriptExecuteStatus, InternalSetupScriptExecuteStatus, InternalTerminateReason, RunUnitQuery, RunUnitRequest, SignalRequest, UnitExecuteStatus, }, target_runner::TargetRunner, @@ -573,6 +574,269 @@ impl<'a> ExecutorContext<'a> { }) } + /// Run a pre-timeoutx script in its own process. + #[instrument(level = "debug", skip(self, resp_tx, req_rx))] + async fn run_pre_timeout_script<'b>( + &self, + script: PreTimeoutScriptPacket<'a, 'b>, + resp_tx: &UnboundedSender>, + req_rx: &mut UnboundedReceiver>, + ) { + let test_instance = script.test.test_instance; + let _ = resp_tx.send(ExecutorEvent::PreTimeoutScriptStarted { test_instance }); + + let mut stopwatch = crate::time::stopwatch(); + + let status = match self + .run_pre_timeout_script_inner(script.clone(), &mut stopwatch, resp_tx, req_rx) + .await + { + Ok(status) => status, + Err(error) => InternalPreTimeoutScriptExecuteStatus { + slow_after: None, + output: ChildExecutionOutput::StartError(error), + result: ExecutionResult::ExecFail, + stopwatch_end: stopwatch.snapshot(), + }, + }; + + let _ = resp_tx.send(ExecutorEvent::PreTimeoutScriptFinished { + test_instance, + script_id: script.script_id, + config: script.config, + status: status.into_external(), + }); + } + + #[instrument(level = "debug", skip(self, resp_tx, req_rx))] + async fn run_pre_timeout_script_inner<'b>( + &self, + script: PreTimeoutScriptPacket<'a, 'b>, + stopwatch: &mut StopwatchStart, + resp_tx: &UnboundedSender>, + req_rx: &mut UnboundedReceiver>, + ) -> Result { + // XXX: this function is 95% the same as run_setup_script_inner. Do we + // need to try to factor out a shared helper method? + + let mut cmd = script.make_command( + &self.double_spawn, + &self.test_list, + &script.test.test_instance, + )?; + let command_mut = cmd.command_mut(); + + // If creating a job fails, we might be on an old system. Ignore this -- job objects are a + // best-effort thing. + let job = super::os::Job::create().ok(); + + // For now, pre-timeout scripts always capture stdout and stderr. + command_mut.stdout(std::process::Stdio::piped()); + command_mut.stderr(std::process::Stdio::piped()); + + let mut child = cmd + .spawn() + .map_err(|error| ChildStartError::Spawn(Arc::new(error)))?; + let child_pid = child + .id() + .expect("child has never been polled so must return a PID"); + + // If assigning the child to the job fails, ignore this. This can happen if the process has + // exited. + let _ = super::os::assign_process_to_job(&child, job.as_ref()); + + let mut status: Option = None; + let slow_timeout = script + .config + .slow_timeout + .unwrap_or(self.profile.slow_timeout()); + let leak_timeout = script + .config + .leak_timeout + .unwrap_or(self.profile.leak_timeout()); + + let mut interval_sleep = std::pin::pin!(crate::time::pausable_sleep(slow_timeout.period)); + + let mut timeout_hit = 0; + + let child_fds = ChildFds::new_split(child.stdout.take(), child.stderr.take()); + let mut child_acc = ChildAccumulator::new(child_fds); + + let mut cx = UnitContext { + packet: UnitPacket::PreTimeoutScript(script.clone()), + slow_after: None, + }; + + let (res, leaked) = { + let res = loop { + tokio::select! { + () = child_acc.fill_buf(), if !child_acc.fds.is_done() => {} + res = child.wait() => { + // The pre-timeout script finished executing. + break res; + } + _ = &mut interval_sleep, if status.is_none() => { + // Mark the script as slow. + cx.slow_after = Some(slow_timeout.period); + + timeout_hit += 1; + let will_terminate = if let Some(terminate_after) = slow_timeout.terminate_after { + NonZeroUsize::new(timeout_hit as usize) + .expect("timeout_hit was just incremented") + >= terminate_after + } else { + false + }; + + if !slow_timeout.grace_period.is_zero() { + let _ = resp_tx.send(script.slow_event( + // Pass in the slow timeout period times timeout_hit, since + // stopwatch.elapsed() tends to be slightly longer. + timeout_hit * slow_timeout.period, + will_terminate.then_some(slow_timeout.grace_period), + )); + } + + if will_terminate { + // Attempt to terminate the slow script. As there is + // a race between shutting down a slow test and its + // own completion, we silently ignore errors to + // avoid printing false warnings. + // + // The return result of terminate_child is not used + // here, since it is always marked as a timeout. + _ = super::os::terminate_child( + &cx, + &mut child, + &mut child_acc, + InternalTerminateReason::Timeout, + stopwatch, + req_rx, + job.as_ref(), + slow_timeout.grace_period, + ).await; + status = Some(ExecutionResult::Timeout); + if slow_timeout.grace_period.is_zero() { + break child.wait().await; + } + // Don't break here to give the wait task a chance to finish. + } else { + interval_sleep.as_mut().reset_last_duration(); + } + } + recv = req_rx.recv() => { + // The sender stays open longer than the whole loop, and the buffer is big + // enough for all messages ever sent through this channel, so a RecvError + // should never happen. + let req = recv.expect("a RecvError should never happen here"); + + match req { + RunUnitRequest::Signal(req) => { + #[cfg_attr(not(windows), expect(unused_variables))] + let res = handle_signal_request( + &cx, + &mut child, + &mut child_acc, + req, + stopwatch, + interval_sleep.as_mut(), + req_rx, + job.as_ref(), + slow_timeout.grace_period + ).await; + + // On Unix, the signal the process exited with + // will be picked up by child.wait. On Windows, + // termination by job object will show up as + // exit code 1 -- we need to be clearer about + // that in the UI. + // + // TODO: Can we do something useful with res on + // Unix? For example, it's possible that the + // signal we send is not the same as the signal + // the child exits with. This might be a good + // thing to store in whatever test event log we + // end up building. + #[cfg(windows)] + { + if matches!( + res, + HandleSignalResult::Terminated(super::TerminateChildResult::Killed) + ) { + status = Some(ExecutionResult::Fail { + abort_status: Some(AbortStatus::JobObject), + leaked: false, + }); + } + } + } + RunUnitRequest::OtherCancel => { + // Ignore non-signal cancellation requests -- + // let the script finish. + } + RunUnitRequest::Query(RunUnitQuery::GetInfo(sender)) => { + _ = sender.send(script.info_response( + UnitState::Running { + pid: child_pid, + time_taken: stopwatch.snapshot().active, + slow_after: cx.slow_after, + }, + child_acc.snapshot_in_progress(UnitKind::WAITING_ON_SCRIPT_MESSAGE), + )); + } + } + } + } + }; + + // Build a tentative status using status and the exit status. + let tentative_status = status.or_else(|| { + res.as_ref() + .ok() + .map(|res| create_execution_result(*res, &child_acc.errors, false)) + }); + + let leaked = detect_fd_leaks( + &cx, + child_pid, + &mut child_acc, + tentative_status, + leak_timeout, + stopwatch, + req_rx, + ) + .await; + + (res, leaked) + }; + + let exit_status = match res { + Ok(exit_status) => Some(exit_status), + Err(err) => { + child_acc.errors.push(ChildFdError::Wait(Arc::new(err))); + None + } + }; + + let exit_status = exit_status.expect("None always results in early return"); + + let exec_result = status + .unwrap_or_else(|| create_execution_result(exit_status, &child_acc.errors, leaked)); + + let errors: Vec<_> = child_acc.errors.into_iter().map(ChildError::from).collect(); + + Ok(InternalPreTimeoutScriptExecuteStatus { + slow_after: cx.slow_after, + output: ChildExecutionOutput::Output { + result: Some(exec_result), + output: child_acc.output.freeze(), + errors: ErrorList::new(UnitKind::WAITING_ON_SCRIPT_MESSAGE, errors), + }, + result: exec_result, + stopwatch_end: stopwatch.snapshot(), + }) + } + /// Run an individual test in its own process. #[instrument(level = "debug", skip(self, resp_tx, req_rx))] async fn run_test( @@ -654,6 +918,7 @@ impl<'a> ExecutorContext<'a> { let mut status: Option = None; let slow_timeout = test.settings.slow_timeout(); let leak_timeout = test.settings.leak_timeout(); + let pre_timeout_script = self.profile.pre_timeout_script(&test.test_instance); // Use a pausable_sleep rather than an interval here because it's much // harder to pause and resume an interval. @@ -697,6 +962,19 @@ impl<'a> ExecutorContext<'a> { } if will_terminate { + if let Some(script) = &pre_timeout_script { + let packet = PreTimeoutScriptPacket { + script_id: script.id.clone(), + test: test.clone(), + test_pid: child_pid, + test_stopwatch: stopwatch, + test_slow_after: slow_timeout.period, + test_output: child_acc.snapshot_in_progress(UnitKind::WAITING_ON_TEST_MESSAGE), + config: script.config, + }; + self.run_pre_timeout_script(packet, resp_tx, req_rx).await; + } + // Attempt to terminate the slow test. As there is a // race between shutting down a slow test and its // own completion, we silently ignore errors to @@ -905,16 +1183,16 @@ impl Iterator for BackoffIter { /// Either a test or a setup script, along with information about how long the /// test took. -pub(super) struct UnitContext<'a> { - packet: UnitPacket<'a>, +pub(super) struct UnitContext<'a, 'b> { + packet: UnitPacket<'a, 'b>, // TODO: This is a bit of a mess. It isn't clear where this kind of state // should live -- many parts of the request-response system need various // pieces of this code. slow_after: Option, } -impl<'a> UnitContext<'a> { - pub(super) fn packet(&self) -> &UnitPacket<'a> { +impl<'a, 'b> UnitContext<'a, 'b> { + pub(super) fn packet(&self) -> &UnitPacket<'a, 'b> { &self.packet } @@ -925,21 +1203,23 @@ impl<'a> UnitContext<'a> { ) -> InfoResponse<'a> { match &self.packet { UnitPacket::SetupScript(packet) => packet.info_response(state, output), + UnitPacket::PreTimeoutScript(packet) => packet.info_response(state, output), UnitPacket::Test(packet) => packet.info_response(state, output), } } } #[derive(Clone, Debug)] -pub(super) enum UnitPacket<'a> { +pub(super) enum UnitPacket<'a, 'b> { SetupScript(SetupScriptPacket<'a>), + PreTimeoutScript(PreTimeoutScriptPacket<'a, 'b>), Test(TestPacket<'a>), } -impl UnitPacket<'_> { +impl UnitPacket<'_, '_> { pub(super) fn kind(&self) -> UnitKind { match self { - Self::SetupScript(_) => UnitKind::Script, + Self::SetupScript(_) | Self::PreTimeoutScript(_) => UnitKind::Script, Self::Test(_) => UnitKind::Test, } } @@ -989,7 +1269,7 @@ impl<'a> TestPacket<'a> { #[derive(Clone, Debug)] pub(super) struct SetupScriptPacket<'a> { script_id: ScriptId, - config: &'a ScriptConfig, + config: &'a SetupScriptConfig, } impl<'a> SetupScriptPacket<'a> { @@ -1026,6 +1306,54 @@ impl<'a> SetupScriptPacket<'a> { } } +#[derive(Clone, Debug)] +pub(super) struct PreTimeoutScriptPacket<'a, 'b> { + script_id: ScriptId, + test: TestPacket<'a>, + test_pid: u32, + test_stopwatch: &'b StopwatchStart, + test_slow_after: Duration, + test_output: ChildExecutionOutput, + config: &'a PreTimeoutScriptConfig, +} + +impl<'a, 'b> PreTimeoutScriptPacket<'a, 'b> { + /// Turns self into a command that can be executed. + fn make_command( + &self, + double_spawn: &DoubleSpawnInfo, + test_list: &TestList<'_>, + test_instance: &TestInstance<'_>, + ) -> Result { + PreTimeoutScriptCommand::new(self.config, double_spawn, test_list, test_instance) + } + + fn slow_event(&self, elapsed: Duration, will_terminate: Option) -> ExecutorEvent<'a> { + ExecutorEvent::PreTimeoutScriptSlow { + test_instance: self.test.test_instance, + elapsed, + will_terminate, + } + } + + pub(super) fn info_response( + &self, + state: UnitState, + output: ChildExecutionOutput, + ) -> InfoResponse<'a> { + // The provided state is about the pre-timeout script, so we need to wrap + // it with the state of the test unit. + let state = UnitState::PreTimeout { + pid: self.test_pid, + time_taken: self.test_stopwatch.snapshot().active, + slow_after: self.test_slow_after, + script_state: Box::new(state), + script_output: output, + }; + self.test.info_response(state, self.test_output.clone()) + } +} + /// Drains the request receiver of any messages. fn drain_req_rx<'a>( mut receiver: UnboundedReceiver>, @@ -1130,7 +1458,7 @@ async fn handle_delay_between_attempts<'a>( /// could do more sophisticated checks around e.g. if any processes with the /// same PGID are around. async fn detect_fd_leaks<'a>( - cx: &UnitContext<'a>, + cx: &UnitContext<'a, '_>, child_pid: u32, child_acc: &mut ChildAccumulator, tentative_result: Option, @@ -1202,7 +1530,7 @@ async fn detect_fd_leaks<'a>( // can cause more harm than good. #[expect(clippy::too_many_arguments)] async fn handle_signal_request<'a>( - cx: &UnitContext<'a>, + cx: &UnitContext<'a, '_>, child: &mut Child, child_acc: &mut ChildAccumulator, req: SignalRequest, diff --git a/nextest-runner/src/runner/internal_events.rs b/nextest-runner/src/runner/internal_events.rs index bdd22d20d24..da74ca7029b 100644 --- a/nextest-runner/src/runner/internal_events.rs +++ b/nextest-runner/src/runner/internal_events.rs @@ -9,12 +9,12 @@ use super::{SetupScriptPacket, TestPacket}; use crate::{ - config::{ScriptConfig, ScriptId}, + config::{PreTimeoutScriptConfig, ScriptId, SetupScriptConfig}, list::TestInstance, reporter::{ events::{ - ExecuteStatus, ExecutionResult, InfoResponse, RetryData, SetupScriptEnvMap, - SetupScriptExecuteStatus, UnitState, + ExecuteStatus, ExecutionResult, InfoResponse, PreTimeoutScriptExecuteStatus, RetryData, + SetupScriptEnvMap, SetupScriptExecuteStatus, UnitState, }, TestOutputDisplay, }, @@ -41,7 +41,7 @@ use tokio::{ pub(super) enum ExecutorEvent<'a> { SetupScriptStarted { script_id: ScriptId, - config: &'a ScriptConfig, + config: &'a SetupScriptConfig, index: usize, total: usize, // See the note in the `Started` variant. @@ -49,13 +49,13 @@ pub(super) enum ExecutorEvent<'a> { }, SetupScriptSlow { script_id: ScriptId, - config: &'a ScriptConfig, + config: &'a SetupScriptConfig, elapsed: Duration, will_terminate: Option, }, SetupScriptFinished { script_id: ScriptId, - config: &'a ScriptConfig, + config: &'a SetupScriptConfig, index: usize, total: usize, status: SetupScriptExecuteStatus, @@ -80,6 +80,20 @@ pub(super) enum ExecutorEvent<'a> { elapsed: Duration, will_terminate: Option, }, + PreTimeoutScriptStarted { + test_instance: TestInstance<'a>, + }, + PreTimeoutScriptSlow { + test_instance: TestInstance<'a>, + elapsed: Duration, + will_terminate: Option, + }, + PreTimeoutScriptFinished { + test_instance: TestInstance<'a>, + script_id: ScriptId, + config: &'a PreTimeoutScriptConfig, + status: PreTimeoutScriptExecuteStatus, + }, AttemptFailedWillRetry { test_instance: TestInstance<'a>, failure_output: TestOutputDisplay, @@ -179,6 +193,25 @@ impl InternalSetupScriptExecuteStatus<'_> { } } +pub(super) struct InternalPreTimeoutScriptExecuteStatus { + pub(super) slow_after: Option, + pub(super) output: ChildExecutionOutput, + pub(super) result: ExecutionResult, + pub(super) stopwatch_end: StopwatchSnapshot, +} + +impl InternalPreTimeoutScriptExecuteStatus { + pub(super) fn into_external(self) -> PreTimeoutScriptExecuteStatus { + PreTimeoutScriptExecuteStatus { + output: self.output, + result: self.result, + start_time: self.stopwatch_end.start_time.fixed_offset(), + time_taken: self.stopwatch_end.active, + is_slow: self.slow_after.is_some(), + } + } +} + /// Events sent from the dispatcher to individual unit execution tasks. #[derive(Clone, Debug)] pub(super) enum RunUnitRequest<'a> { diff --git a/nextest-runner/src/runner/unix.rs b/nextest-runner/src/runner/unix.rs index 4c01e8f3028..de2d38b14bf 100644 --- a/nextest-runner/src/runner/unix.rs +++ b/nextest-runner/src/runner/unix.rs @@ -78,7 +78,7 @@ pub(super) fn raise_stop() { // Windows) or grace_period (only relevant on Unix) together. #[expect(clippy::too_many_arguments)] pub(super) async fn terminate_child<'a>( - cx: &UnitContext<'a>, + cx: &UnitContext<'a, '_>, child: &mut Child, child_acc: &mut ChildAccumulator, reason: InternalTerminateReason, From 823eb6e6ad7a1c41fbb9e9d7b4fe789d358be216 Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Wed, 8 Jan 2025 21:53:32 +0000 Subject: [PATCH 10/13] [squashme] fix existing tests --- .config/nextest.toml | 2 +- fixtures/nextest-tests/.config/nextest.toml | 4 +- nextest-runner/src/config/config_impl.rs | 49 ++++++++++++--------- nextest-runner/src/config/scripts.rs | 36 +++++++-------- 4 files changed, 49 insertions(+), 42 deletions(-) diff --git a/.config/nextest.toml b/.config/nextest.toml index dcc6801a144..4ab2d5b575a 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -62,7 +62,7 @@ max-threads = 8 [test-groups.unused-group] max-threads = 8 -[script.build-seed-archive] +[script.setup.build-seed-archive] # This command builds a seed archive that's used by the integration-tests # package. This archive is not currently used by the older nextest-runner # integration framework, but that should really go away at some point. diff --git a/fixtures/nextest-tests/.config/nextest.toml b/fixtures/nextest-tests/.config/nextest.toml index 6f383e74746..91836c110f7 100644 --- a/fixtures/nextest-tests/.config/nextest.toml +++ b/fixtures/nextest-tests/.config/nextest.toml @@ -91,8 +91,8 @@ max-threads = 4 [test-groups.unused] max-threads = 20 -[script.my-script-unix] +[script.setup.my-script-unix] command = './scripts/my-script.sh' -[script.my-script-windows] +[script.setup.my-script-windows] command = 'cmd /c "scripts\\my-script.bat"' diff --git a/nextest-runner/src/config/config_impl.rs b/nextest-runner/src/config/config_impl.rs index 6aa4e430299..7f645d162c1 100644 --- a/nextest-runner/src/config/config_impl.rs +++ b/nextest-runner/src/config/config_impl.rs @@ -218,7 +218,7 @@ impl NextestConfig { let mut compiled = CompiledByProfile::for_default_config(); let mut known_groups = BTreeSet::new(); - let mut known_scripts = BTreeSet::new(); + let mut known_scripts = BTreeMap::new(); // Next, merge in tool configs. for ToolConfigFile { config_file, tool } in tool_config_files_rev { @@ -290,7 +290,7 @@ impl NextestConfig { experimental: &BTreeSet, unknown_callback: &mut impl FnMut(&Utf8Path, Option<&str>, &BTreeSet), known_groups: &mut BTreeSet, - known_scripts: &mut BTreeSet, + known_scripts: &mut BTreeMap, ) -> Result<(), ConfigParseError> { // Try building default builder + this file to get good error attribution and handle // overrides additively. @@ -390,7 +390,10 @@ impl NextestConfig { return Err(ConfigParseError::new(config_file, tool, kind)); } - known_scripts.extend(valid_scripts); + known_scripts.extend(valid_scripts.into_iter().map(|id| { + let script_type = this_config.scripts.script_type(&id); + (id, script_type) + })); let this_config = this_config.into_config_impl(); @@ -475,23 +478,25 @@ impl NextestConfig { )); } for script in scripts { - if !known_scripts.contains(script) { - unknown_script_errors.push(UnknownConfigScriptError { - profile_name: profile_name.to_owned(), - name: script.clone(), - }); - } - let actual_script_type = this_config.scripts.script_type(script); - if actual_script_type != script_type { - return Err(ConfigParseError::new( - config_file, - tool, - ConfigParseErrorKind::WrongConfigScriptType { - script: script.clone(), - attempted: script_type, - actual: actual_script_type, - }, - )); + match known_scripts.get(script) { + None => { + unknown_script_errors.push(UnknownConfigScriptError { + profile_name: profile_name.to_owned(), + name: script.clone(), + }); + } + Some(actual_script_type) if *actual_script_type != script_type => { + return Err(ConfigParseError::new( + config_file, + tool, + ConfigParseErrorKind::WrongConfigScriptType { + script: script.clone(), + attempted: script_type, + actual: actual_script_type.clone(), + }, + )); + } + Some(_) => (), } } @@ -526,7 +531,7 @@ impl NextestConfig { // If there were any unknown scripts, error out. if !unknown_script_errors.is_empty() { - let known_scripts = known_scripts.iter().cloned().collect(); + let known_scripts = known_scripts.keys().cloned().collect(); return Err(ConfigParseError::new( config_file, tool, @@ -1088,8 +1093,10 @@ impl CustomProfileImpl { pub struct Scripts { // These maps are ordered because scripts are used in the order they're defined. /// The setup scripts defined in a profile. + #[serde(default)] pub setup: IndexMap, /// The pre-timeout scripts defined in a profile. + #[serde(default)] pub pre_timeout: IndexMap, } diff --git a/nextest-runner/src/config/scripts.rs b/nextest-runner/src/config/scripts.rs index af0a196b39e..21e500f5fd3 100644 --- a/nextest-runner/src/config/scripts.rs +++ b/nextest-runner/src/config/scripts.rs @@ -761,14 +761,14 @@ mod tests { # order defined below. setup = ["baz", "foo", "@tool:my-tool:toolscript"] - [script.foo] + [script.setup.foo] command = "command foo" - [script.bar] + [script.setup.bar] command = ["cargo", "run", "-p", "bar"] slow-timeout = { period = "60s", terminate-after = 2 } - [script.baz] + [script.setup.baz] command = "baz" slow-timeout = "1s" leak-timeout = "1s" @@ -778,7 +778,7 @@ mod tests { }; let tool_config_contents = indoc! {r#" - [script.'@tool:my-tool:toolscript'] + [script.setup.'@tool:my-tool:toolscript'] command = "tool-command" "# }; @@ -892,7 +892,7 @@ mod tests { #[test_case( indoc! {r#" - [script.foo] + [script.setup.foo] command = "" "#}, "invalid value: string \"\", expected a Unix shell command or a list of arguments" @@ -901,7 +901,7 @@ mod tests { )] #[test_case( indoc! {r#" - [script.foo] + [script.setup.foo] command = [] "#}, "invalid length 0, expected a Unix shell command or a list of arguments" @@ -910,15 +910,15 @@ mod tests { )] #[test_case( indoc! {r#" - [script.foo] + [script.setup.foo] "#}, - "script.foo: missing field `command`" + "script.setup.foo: missing field `command`" ; "missing command" )] #[test_case( indoc! {r#" - [script.foo] + [script.setup.foo] command = "my-command" slow-timeout = 34 "#}, @@ -928,7 +928,7 @@ mod tests { )] #[test_case( indoc! {r#" - [script.'@tool:foo'] + [script.setup.'@tool:foo'] command = "my-command" "#}, r#"invalid configuration script name: tool identifier not of the form "@tool:tool-name:identifier": `@tool:foo`"# @@ -937,7 +937,7 @@ mod tests { )] #[test_case( indoc! {r#" - [script.'#foo'] + [script.setup.'#foo'] command = "my-command" "#}, r"invalid configuration script name: invalid identifier `#foo`" @@ -967,7 +967,7 @@ mod tests { #[test_case( indoc! {r#" - [script.foo] + [script.setup.foo] command = "my-command" [[profile.default.scripts]] @@ -983,7 +983,7 @@ mod tests { )] #[test_case( indoc! {r#" - [script.foo] + [script.setup.foo] command = "my-command" [[profile.default.scripts]] @@ -1000,7 +1000,7 @@ mod tests { )] #[test_case( indoc! {r#" - [script.foo] + [script.setup.foo] command = "my-command" [[profile.default.scripts]] @@ -1019,7 +1019,7 @@ mod tests { )] #[test_case( indoc! {r#" - [script.foo] + [script.setup.foo] command = "my-command" [[profile.ci.overrides]] @@ -1092,7 +1092,7 @@ mod tests { #[test_case( indoc! {r#" - [script.'@tool:foo:bar'] + [script.setup.'@tool:foo:bar'] command = "my-command" [[profile.ci.overrides]] @@ -1133,7 +1133,7 @@ mod tests { #[test_case( indoc! {r#" - [script.'blarg'] + [script.setup.'blarg'] command = "my-command" [[profile.ci.overrides]] @@ -1183,7 +1183,7 @@ mod tests { #[test_case( indoc! {r#" - [script.foo] + [script.setup.foo] command = "my-command" [[profile.default.scripts]] From ed1df4060ba9ec3494569d8122b6c6aa157711f7 Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Wed, 8 Jan 2025 21:58:20 +0000 Subject: [PATCH 11/13] [squashme] update pre-timeout script env var naming --- nextest-runner/src/config/scripts.rs | 2 ++ site/src/docs/scripts/pre-timeout.md | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/nextest-runner/src/config/scripts.rs b/nextest-runner/src/config/scripts.rs index 21e500f5fd3..0620025c47c 100644 --- a/nextest-runner/src/config/scripts.rs +++ b/nextest-runner/src/config/scripts.rs @@ -308,6 +308,8 @@ impl PreTimeoutScriptCommand { // This environment variable is set to indicate that tests are being run under nextest. .env("NEXTEST", "1"); + // XXX: set special pre-timeout script environment variables. + apply_ld_dyld_env(&mut cmd, test_list.updated_dylib_path()); let double_spawn = double_spawn.spawn_context(); diff --git a/site/src/docs/scripts/pre-timeout.md b/site/src/docs/scripts/pre-timeout.md index d72c8190226..9ed71871162 100644 --- a/site/src/docs/scripts/pre-timeout.md +++ b/site/src/docs/scripts/pre-timeout.md @@ -64,8 +64,8 @@ Nextest executes pre-timeout scripts with the same working directory as the test * **`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. +* **`NEXTEST_PRE_TIMEOUT_TEST_PACKAGE`**: the name of the package (crate) containing the test. +* **`NEXTEST_PRE_TIMEOUT_TEST_BINARY`**: the name of the binary hosting the test. +* **`NEXTEST_PRE_TIMEOUT_TEST_BINARY_KIND`**: the kind of the binary hosting the test. From 38dac56e919ef7cc31fff22a07d8887401e23ba8 Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Wed, 8 Jan 2025 22:11:00 +0000 Subject: [PATCH 12/13] [squashme] ensure pre-timeout happens before terminating --- nextest-runner/src/runner/executor.rs | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/nextest-runner/src/runner/executor.rs b/nextest-runner/src/runner/executor.rs index 16ceecfb32c..ed9ed813bcd 100644 --- a/nextest-runner/src/runner/executor.rs +++ b/nextest-runner/src/runner/executor.rs @@ -952,15 +952,6 @@ impl<'a> ExecutorContext<'a> { false }; - if !slow_timeout.grace_period.is_zero() { - let _ = resp_tx.send(test.slow_event( - // Pass in the slow timeout period times timeout_hit, since - // stopwatch.elapsed() tends to be slightly longer. - timeout_hit * slow_timeout.period, - will_terminate.then_some(slow_timeout.grace_period), - )); - } - if will_terminate { if let Some(script) = &pre_timeout_script { let packet = PreTimeoutScriptPacket { @@ -974,7 +965,22 @@ impl<'a> ExecutorContext<'a> { }; self.run_pre_timeout_script(packet, resp_tx, req_rx).await; } + } + // Send the will-terminate event *after* the pre-timeout + // script has completed (if it exists), so that the user + // observes the state transitions in this order: running + // slow -> pre-timeout -> terminating. + if !slow_timeout.grace_period.is_zero() { + let _ = resp_tx.send(test.slow_event( + // Pass in the slow timeout period times timeout_hit, since + // stopwatch.elapsed() tends to be slightly longer. + timeout_hit * slow_timeout.period, + will_terminate.then_some(slow_timeout.grace_period), + )); + } + + if will_terminate { // Attempt to terminate the slow test. As there is a // race between shutting down a slow test and its // own completion, we silently ignore errors to From 121f268a3cda8daa2e0632ade6879cb90b4767c2 Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Wed, 8 Jan 2025 22:52:43 +0000 Subject: [PATCH 13/13] [squashme] set special env vars when executing pre-timeout script --- nextest-runner/src/config/scripts.rs | 9 ++++++++- nextest-runner/src/runner/executor.rs | 4 +++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/nextest-runner/src/config/scripts.rs b/nextest-runner/src/config/scripts.rs index 0620025c47c..38dbb21b374 100644 --- a/nextest-runner/src/config/scripts.rs +++ b/nextest-runner/src/config/scripts.rs @@ -297,6 +297,7 @@ impl PreTimeoutScriptCommand { double_spawn: &DoubleSpawnInfo, test_list: &TestList<'_>, test_instance: &TestInstance<'_>, + test_pid: u32, ) -> Result { let mut cmd = create_command(config.program().to_owned(), config.args(), double_spawn); @@ -306,7 +307,13 @@ impl PreTimeoutScriptCommand { cmd.current_dir(&test_instance.suite_info.cwd) // This environment variable is set to indicate that tests are being run under nextest. - .env("NEXTEST", "1"); + .env("NEXTEST", "1") + .env("NEXTEST_PRE_TIMEOUT_TEST_PID", test_pid.to_string()) + .env("NEXTEST_PRE_TIMEOUT_TEST_NAME", test_instance.name) + .env("NEXTEST_PRE_TIMEOUT_TEST_BINARY_ID", test_instance.suite_info.binary_id.as_str()) + .env("NEXTEST_PRE_TIMEOUT_TEST_PACKAGE", test_instance.suite_info.package.name()) + .env("NEXTEST_PRE_TIMEOUT_TEST_BINARY", &test_instance.suite_info.binary_name) + .env("NEXTEST_PRE_TIMEOUT_TEST_BINARY_KIND", test_instance.suite_info.kind.as_str()); // XXX: set special pre-timeout script environment variables. diff --git a/nextest-runner/src/runner/executor.rs b/nextest-runner/src/runner/executor.rs index ed9ed813bcd..95ea372c080 100644 --- a/nextest-runner/src/runner/executor.rs +++ b/nextest-runner/src/runner/executor.rs @@ -623,6 +623,7 @@ impl<'a> ExecutorContext<'a> { &self.double_spawn, &self.test_list, &script.test.test_instance, + script.test_pid, )?; let command_mut = cmd.command_mut(); @@ -1330,8 +1331,9 @@ impl<'a, 'b> PreTimeoutScriptPacket<'a, 'b> { double_spawn: &DoubleSpawnInfo, test_list: &TestList<'_>, test_instance: &TestInstance<'_>, + test_pid: u32, ) -> Result { - PreTimeoutScriptCommand::new(self.config, double_spawn, test_list, test_instance) + PreTimeoutScriptCommand::new(self.config, double_spawn, test_list, test_instance, test_pid) } fn slow_event(&self, elapsed: Duration, will_terminate: Option) -> ExecutorEvent<'a> {