[DNM] Add documentation for hypothetical pre-timeout scripts #2018

benesch · 2024-12-26T06:19:42Z

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.

cc @criccomini

In the spirit of documentation driven development, this commit adds docs for the pre-timeout scripts proposed in nextest-rs#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.

codecov · 2024-12-26T06:22:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.93%. Comparing base (eaa5634) to head (a107acf).
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2018      +/-   ##
==========================================
- Coverage   79.94%   79.93%   -0.01%     
==========================================
  Files          97       97              
  Lines       23631    23677      +46     
==========================================
+ Hits        18891    18926      +35     
- Misses       4740     4751      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sunshowers

Thank you for writing this up!

sunshowers · 2024-12-26T22:30:11Z

site/src/docs/configuration/scripts.md

 [script.my-script]
 command = 'my-script.sh'


I think it's probably worth making this script.setup.my-script. I think the scripts are going to have fairly different semantics and settings etc. We can make this change because scripts are currently experimental.

I would still put all script names in a single namespace, though (i.e. not allow any name overlaps) to reduce confusion.

Makes sense! Updated.

sunshowers · 2024-12-26T22:33:17Z

site/src/docs/configuration/scripts.md

+### 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


Not hugely important at the moment, but I think this level of nesting suggests using a different page for pre-timeout scripts. I think there should probably be a scripts/index.md page with an overview and common options, and then separate pages for setup and pre-timeout scripts.

Yeah, I had the same thought. I went ahead with that refactor now, since I think it will help make space for covering some of the finer points of how pre-timeout scripts will work.

sunshowers · 2024-12-26T22:34:13Z

site/src/docs/configuration/scripts.md

+
+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.


Is it worth supporting multiple pre-timeout scripts for a single test at all? That seems like it'll add confusion.

I'm partial to just one script. Users can wrap multiple scripts themselves if need be.

I agree it might be a bit confusing, but it seems hard to not support multiple pre-timeout scripts for the same test with the current rules-based approach for matching scripts to tests. E.g. imagine that team 1 and team 2 set up pre-timeout rules:

[[profile.default.scripts]] filter = 'rdeps(team1-crate)' pre-timeout = 'dump-backtraces' [[profile.default.scripts]] filter = 'rdeps(team2-crate)' pre-timeout = 'notify-sentry'

If someone then introduces a crate that depends on both team1-crate and team2-crate, there are now two pre-timeout scripts applied to the tests in that crate.

So having a well-defined execution order for multiple pre-timeout scripts seemed like a sensible way to handle that situation. The alternative, I guess, would be for nextest to refuse to execute tests whenever the pre-timeout rules resulted in multiple pre-timeout scripts being matched for a single test?

Nextest already has precedence rules for overrides (first matching override wins), so we can just use that.

sunshowers · 2024-12-26T23:37:29Z

site/src/docs/configuration/scripts.md

+  * **`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.


I'd call the last one BINARY_ID. Maybe also pass in the components of the binary ID as separate variables for convenience.

Sure, done! I wasn't quite sure what env var names to use for the binary ID components. I'll tag you in to a new conversation on this point—unfortunately splitting out the markdown files means this discussion is no longer attached to any lines.

sunshowers · 2024-12-26T23:40:41Z

site/src/docs/configuration/scripts.md

@@ -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.


One of the things it would be useful to add here is a practical use case that's outlined. For setup scripts, database setup or preparing artifacts for individual test use are illustrative examples. For pre-timeout scripts, it would be good to mention grabbing stack traces and logs. That kind of use case would also help shape their functionality.

It would also be useful to cover some more of the points from my post:

Should the script be responsible for killing the test, or should nextest do so (after waiting for the script to exit)? I'd lean towards the latter.

Covering this would be useful -- I'm still leaning towards killing the process group anyway, but saying it explicitly would be nice.

Do we still want to allow a grace period (SIGTERM before SIGKILL) in this case?

This is worth mentioning as well. Pre-timeout scripts add a bunch of states to the state machine, because each a unit of work now has a sidecar process involved as well. One approach is to add the pre-timeout script to the process group (Unix) / job object (Windows) -- that would provide fairly clear semantics, I think. The script and the test then live and die together -- the code that waits for the child process to exit can now wait on both processes.

What should the cwd of the script process be?

This should just be the same as the cwd of the test, I'm quite sure.

How should information about the script being run be communicated to the user?

The states presented to the user blow up a bit here.

One of the things that's worth thinking about, I think, is passing through stdout and stderr -- unlike setup scripts, pre-timeout scripts aren't executed serially, so capturing stdout/stderr generally makes a lot of sense.

But! There's also a compelling use case for pre-timeout scripts: to put you into an interactive debugger, effectively acting as a break point. In that case, you do want to pass through stdout and stderr, and you want to keep processing existing tests in the background, but (I think) not start new ones. Even if we don't solve it here, it's worth keeping that case in mind.

Does nextest need to create a directory for the script to write logs and such out to? Even if not necessary, is it a good to have?

I really would like this, personally. That information can then be attached in all sorts of ways, e.g. to JUnit reports.

I think it would also be useful to have a protocol for the pre-timeout script to give information back to nextest -- for example, the list of files written out and/or to include. This can just be a file the script writes to, similar to NEXTEST_ENV for setup scripts.

The exact details for much of this can be outlined in an architecture doc (this is more usage-oriented), but a quick summary of a lot of this would be useful.

Sounds good! I pushed up a revision that begins to tackle these questions. I also split out individual discussions for each of your questions, since this discussion is now on an orphaned hunk of code.

benesch · 2024-12-29T17:17:22Z

Thanks for this great feedback, @sunshowers! Will take an another spin through the docs and start working in answers to the questions you mentioned. I've got some long haul flights coming up this week and next, and this should make for a perfect plane project.

benesch

Finally found the time to get back to this! @sunshowers @criccomini thanks for all the time so far—have a bunch more questions for y'all!

My internet access is quite spotty for the week, but I have a fair bit of free time. I'll start working on the first draft of an implementation of what I've proposed here while I'm offline, and I'll check in on the discussion here as my internet access permits!

benesch · 2025-01-01T15:37:22Z

site/src/docs/configuration/scripts.md

 [script.my-script]
 command = 'my-script.sh'


Makes sense! Updated.

benesch · 2025-01-01T16:08:07Z

site/src/docs/configuration/scripts.md

+### 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


Yeah, I had the same thought. I went ahead with that refactor now, since I think it will help make space for covering some of the finer points of how pre-timeout scripts will work.

benesch · 2025-01-01T16:24:05Z

site/src/docs/configuration/scripts.md

+
+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.


I agree it might be a bit confusing, but it seems hard to not support multiple pre-timeout scripts for the same test with the current rules-based approach for matching scripts to tests. E.g. imagine that team 1 and team 2 set up pre-timeout rules:

[[profile.default.scripts]] filter = 'rdeps(team1-crate)' pre-timeout = 'dump-backtraces' [[profile.default.scripts]] filter = 'rdeps(team2-crate)' pre-timeout = 'notify-sentry'

If someone then introduces a crate that depends on both team1-crate and team2-crate, there are now two pre-timeout scripts applied to the tests in that crate.

So having a well-defined execution order for multiple pre-timeout scripts seemed like a sensible way to handle that situation. The alternative, I guess, would be for nextest to refuse to execute tests whenever the pre-timeout rules resulted in multiple pre-timeout scripts being matched for a single test?

benesch · 2025-01-01T16:25:57Z

site/src/docs/configuration/scripts.md

+  * **`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.


Sure, done! I wasn't quite sure what env var names to use for the binary ID components. I'll tag you in to a new conversation on this point—unfortunately splitting out the markdown files means this discussion is no longer attached to any lines.

benesch · 2025-01-02T04:02:00Z

site/src/docs/scripts/pre-timeout.md

+* **`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.


@sunshowers would love your input on whether these are the right names for the _BINARY_ID_* environment variables.

I think I'd match the filterset DSL. I'd change:

NEXTEST_PRE_TIMEOUT_TEST_BINARY_ID_PACKAGE_NAME -> NEXTEST_PRE_TIMEOUT_TEST_PACKAGE

NEXTEST_PRE_TIMEOUT_TEST_BINARY_ID_NAME -> NEXTEST_PRE_TIMEOUT_TEST_BINARY

NEXTEST_PRE_TIMEOUT_TEST_BINARY_ID_KIND -> NEXTEST_PRE_TIMEOUT_TEST_BINARY_KIND. I think BINARY_KIND here rather than KIND because the latter is a bit confusing, though I'm not certain of this. What do you think?

I think I'd match the filterset DSL. I'd change:

NEXTEST_PRE_TIMEOUT_TEST_BINARY_ID_PACKAGE_NAME -> NEXTEST_PRE_TIMEOUT_TEST_PACKAGE

NEXTEST_PRE_TIMEOUT_TEST_BINARY_ID_NAME -> NEXTEST_PRE_TIMEOUT_TEST_BINARY

NEXTEST_PRE_TIMEOUT_TEST_BINARY_ID_KIND -> NEXTEST_PRE_TIMEOUT_TEST_BINARY_KIND. I think BINARY_KIND here rather than KIND because the latter is a bit confusing, though I'm not certain of this. What do you think?

Makes sense to me!

benesch · 2025-01-02T04:03:45Z

site/src/docs/scripts/pre-timeout.md

+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.


Beginning to address this question:

Should the script be responsible for killing the test, or should nextest do so (after waiting for the script to exit)? I'd lean towards the latter.

Covering this would be useful -- I'm still leaning towards killing the process group anyway, but saying it explicitly would be nice.

and this one:

Do we still want to allow a grace period (SIGTERM before SIGKILL) in this case?

This is worth mentioning as well. Pre-timeout scripts add a bunch of states to the state machine, because each a unit of work now has a sidecar process involved as well. One approach is to add the pre-timeout script to the process group (Unix) / job object (Windows) -- that would provide fairly clear semantics, I think. The script and the test then live and die together -- the code that waits for the child process to exit can now wait on both processes.

I think I may need to get further into the code to really understand the complexity here, but it's not immediately obvious to me why we need to couple the script and test lifetimes together or put the processes in the same pgrp. Naively it seems straightforward to just apply the graceful termination logic to the pre-timeout script and then move on to apply the graceful termination logic to the test.

It's definitely possible to do that, but my concern is just that it blows up the state machine even further. The state machine for an individual test already has on the order of 40-50 states today once you consider all the combinations of select branches that are or aren't done at any moment.

Adding a "start script -> read stdout/stderr from script -> wait on script + test to exit -> check for the script leaking handles" already blows it up quite a bit, though the benefits outweigh the complexity.

Adding "SIGTERM to script -> maybe SIGKILL -> script done -> SIGTERM to test -> maybe SIGKILL -> test done" adds even more states to the state machine, and I'm not sure this pays its way compared to killing the script and the test at the same time.

But, I agree that this decision can be made based on how complex the implementation gets.

It wasn't terribly difficult to add a run_pre_timeout_script that handles slow timeouts/graceful termination/leak checking just like running tests and running setup scripts. Two open questions though:

whether this results in the right end-user experience: [DNM] Add documentation for hypothetical pre-timeout scripts #2018 (comment)

whether this results in too much code duplication: [DNM] Add documentation for hypothetical pre-timeout scripts #2018 (comment)

benesch · 2025-01-02T04:04:48Z

site/src/docs/scripts/pre-timeout.md

+
+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:


Added a clause here specifying the CWD:

What should the cwd of the script process be?

This should just be the same as the cwd of the test, I'm quite sure.

benesch · 2025-01-02T04:05:56Z

site/src/docs/scripts/pre-timeout.md

+
+Pre-timeout scripts do not support additional configuration options.
+
+Notably, pre-timeout scripts always capture stdout and stderr. Support for not capturing stdout and stderr may be added in the future in order to support use cases like interactive debugging of a hung test.


Starting to address this:

How should information about the script being run be communicated to the user?

The states presented to the user blow up a bit here.

One of the things that's worth thinking about, I think, is passing through stdout and stderr -- unlike setup scripts, pre-timeout scripts aren't executed serially, so capturing stdout/stderr generally makes a lot of sense.

But! There's also a compelling use case for pre-timeout scripts: to put you into an interactive debugger, effectively acting as a break point. In that case, you do want to pass through stdout and stderr, and you want to keep processing existing tests in the background, but (I think) not start new ones. Even if we don't solve it here, it's worth keeping that case in mind.

I was thinking to keep things simple for the initial implementation and force that stdout/stderr are captured. @sunshowers does that make sense to you?

How should information about the script being run be communicated to the user?

Flagging that I haven't forgotten about this! I think I'll need to get a little further along in the implementation before I can answer this question, though. Happy to take any advice you have in the meantime, though, @sunshowers, if there's a particular UX you're imagining!

I was thinking to keep things simple for the initial implementation and force that stdout/stderr are captured. @sunshowers does that make sense to you?

Yeah, this is ok, keep it simple for now.

Flagging that I haven't forgotten about this! I think I'll need to get a little further along in the implementation before I can answer this question, though. Happy to take any advice you have in the meantime, though, @sunshowers, if there's a particular UX you're imagining!

So there's several points to surface user information here.

At the moment the test has timed out:

Today we print out a TERMINATING line:

nextest/nextest-runner/src/reporter/displayer/imp.rs

Lines 347 to 364 in e595f3f

} else if *will_terminate {

let (required_status_level, style) = if retry_data.is_last_attempt() {

(StatusLevel::Fail, self.styles.fail)

} else {

(StatusLevel::Retry, self.styles.retry)

};

if retry_data.total_attempts > 1

&& self.status_levels.status_level > required_status_level

{

write!(

writer,

"{:>12} ",

format!("TRY {} TRMNTG", retry_data.attempt).style(style)

)?;

} else {

write!(writer, "{:>12} ", "TERMINATING".style(style))?;

};

}

If there's a pre-timeout script I'd add "running pre-timeout script foo against test" on a second line. We have a format for this already, used for Windows abort codes (it's aligned so that the - is in the same column as ])

nextest/nextest-runner/src/reporter/displayer/snapshots/nextest_runner__reporter__displayer__imp__windows_tests__stack_violation_code.snap

Line 6 in e595f3f

- with code 0xc00001b2: Invalid access to memory location. (os error 998)

Once the script is done executing:

We should print the test's output, as well as the script's output, with the two being clearly distinguished from each other.

In information queries: (i.e. t, or ctrl-t/SIGINFO where available):

We should say something like "terminating due to timeout" -> "running pre-timeout script foo", and carry around all the details for foo just like we would for any other unit.

To do this we'd need to expand the terminating state to carry this information, I think:

nextest/nextest-runner/src/reporter/events.rs

Lines 1023 to 1045 in e595f3f

/// The current terminating state of a test or script process.

///

/// Part of [`UnitState::Terminating`].

#[derive(Clone, Debug)]

pub struct UnitTerminatingState {

/// The process ID.

pub pid: u32,

/// The amount of time the unit ran for.

pub time_taken: Duration,

/// The reason for the termination.

pub reason: UnitTerminateReason,

/// The method by which the process is being terminated.

pub method: UnitTerminateMethod,

/// How long has been spent waiting for the process to exit.

pub waiting_duration: Duration,

/// How much longer nextest will wait until a kill command is sent to the process.

pub remaining: Duration,

}

Maybe in an Option<Box<T>> field.

Makes sense! I ended up with a slightly different approach than you proposed. Instead of adding an optional pre-timeout field to the Terminating state, I added a new top-level UnitState::PreTimeout state, which itself contains a script_state: UnitState field. This allows the pre-timeout script itself to transition between the standard unit states (running, slow, terminating, exited) with minimal code duplication.

Here's how the output looks in this initial implementation.

Basic statuses:

$ ~/Sites/nextest/target/debug/cargo-nextest nextest run --no-fail-fast info: experimental features enabled: setup-scripts, pre-timeout-scripts Finished `test` profile [unoptimized + debuginfo] target(s) in 0.00s ──────────── Nextest run ID d0f8b05a-5a82-4fa7-9f3c-5cccd3cbccbb with nextest profile: default Starting 2 tests across 1 binary [ 00:00:00] 0/2: SETUP [ 1/1] my-script: ./setup.sh setting up! SETUP PASS [ 0.015s] my-script: ./setup.sh SLOW [> 1.000s] nextest-play tests::it_no_works SLOW [> 1.000s] nextest-play tests::it_works TERMINATING [> 2.000s] nextest-play tests::it_no_works PRETMT nextest-play tests::it_works TIMEOUT [ 2.007s] nextest-play tests::it_no_works ──── STDOUT: nextest-play tests::it_no_works running 1 test PRETMT SLOW [> 1.000s] nextest-play tests::it_works PRETMT PASS [ 1.233s] nextest-play tests::it_works ──── STDOUT: gdb-dump: ./pre-timeout.sh pre-timeout sample stdout ──── STDERR: gdb-dump: ./pre-timeout.sh pre-timeout sample stderr TERMINATING [> 2.000s] nextest-play tests::it_works TIMEOUT [ 3.240s] nextest-play tests::it_works ──── STDOUT: nextest-play tests::it_works running 1 test test sample stdout ──── STDERR: nextest-play tests::it_works test sample stderr ──────────── Summary [ 3.257s] 2 tests run: 0 passed, 2 timed out, 0 skipped TIMEOUT [ 2.007s] nextest-play tests::it_no_works TIMEOUT [ 3.240s] nextest-play tests::it_works error: test run failed

A test with an activated pre-timeout script transitions like so:

Running (not printed)

Slow (printed as SLOW)

Pre-timeout script starting (printed as PRETMT)

Pre-timeout script itself slow (printed as PRETMT SLOW)

Pre-timeout script pass/fail (printed as PRETMT PASS/PRETMT FAIL, followed by the pre-timeout script's stdout/stderr)

Terminating (printed as TERMINATING)

Exit (printed as TIMEOUT, followed by the test's stdout/stderr)

If you request status while the pre-timeout script is executing, you see something like this:

* 1/1: nextest-play tests::it_works status: test running for 2.493s as PID 41610 (marked slow after 1.000s) note: test has reached timeout, pre-timeout script is running: status: script running for 0.485s as PID 41641 stdout: pre-timeout sample stdout stderr: pre-timeout sample stderr

I'm by no means married to any of this. It's just a result of my initial attempt to balance "clear to end users" and "straightforward to implement."

benesch · 2025-01-02T04:07:00Z

site/src/docs/scripts/pre-timeout.md

+* **`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.
+
+<!-- TODO: a protocol for writing script logs to a file and telling nextest to attach them to JUnit reports? -->


Does nextest need to create a directory for the script to write logs and such out to? Even if not necessary, is it a good to have?

I really would like this, personally. That information can then be attached in all sorts of ways, e.g. to JUnit reports.

I think it would also be useful to have a protocol for the pre-timeout script to give information back to nextest -- for example, the list of files written out and/or to include. This can just be a file the script writes to, similar to NEXTEST_ENV for setup scripts.

The exact details for much of this can be outlined in an architecture doc (this is more usage-oriented), but a quick summary of a lot of this would be useful.

@sunshowers are you comfortable deferring this to future work? I'd like to try to keep the initial implementation as focused as possible, and this bit seems a hunk of work that's easy to split off.

Yeah, fine to defer to the future. I just want to make sure we don't box ourselves into making this impossible in the future.

benesch · 2025-01-02T04:09:37Z

site/src/docs/configuration/scripts.md

@@ -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.


Sounds good! I pushed up a revision that begins to tackle these questions. I also split out individual discussions for each of your questions, since this discussion is now on an orphaned hunk of code.

sunshowers · 2025-01-03T22:17:19Z

Thanks for the wait! I've gone through and answered all the questions. I think the design is in generally good shape, and you're asking all the right questions :D

benesch

As promised, I've whipped up an initial implementation! The code here is largely feature complete and works well in my manual testing.

I've not yet written any automated tests. I figured it would be more efficient to defer writing tests until after an initial few rounds of reviews. I didn't want to burn cycles locking in behavior that we wind up wanting to change.

@sunshowers let me know if there's anything I can do to make this easier to review. The PR's gotten fairly large. I've been pushing up my raw commits so that you can use GitHub to skip over commits you've already reviewed. If you'd prefer, I can start squashing/splitting my raw commits into a tidy series of semantic changes; the downside, of course, is that I'd have to force-push the history and you wouldn't as easily be able to see what changed after every force-push.

benesch · 2025-01-08T21:57:08Z

site/src/docs/scripts/pre-timeout.md

+* **`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.


I think I'd match the filterset DSL. I'd change:

NEXTEST_PRE_TIMEOUT_TEST_BINARY_ID_PACKAGE_NAME -> NEXTEST_PRE_TIMEOUT_TEST_PACKAGE

NEXTEST_PRE_TIMEOUT_TEST_BINARY_ID_NAME -> NEXTEST_PRE_TIMEOUT_TEST_BINARY

NEXTEST_PRE_TIMEOUT_TEST_BINARY_ID_KIND -> NEXTEST_PRE_TIMEOUT_TEST_BINARY_KIND. I think BINARY_KIND here rather than KIND because the latter is a bit confusing, though I'm not certain of this. What do you think?

Makes sense to me!

benesch · 2025-01-08T21:59:24Z

nextest-runner/src/config/scripts.rs

+            // 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.


Flagging this outstanding todo as a note to myself.

benesch · 2025-01-08T22:00:20Z

nextest-runner/src/runner/executor.rs

+        // XXX: this function is 95% the same as run_setup_script_inner. Do we
+        // need to try to factor out a shared helper method?


@sunshowers would love your quick take on this question.

95% the same -- what are the differences? Looking at the function itself, yes -- I'd love to see some common code factored out.

benesch · 2025-01-08T22:32:43Z

site/src/docs/scripts/pre-timeout.md

+
+Pre-timeout scripts do not support additional configuration options.
+
+Notably, pre-timeout scripts always capture stdout and stderr. Support for not capturing stdout and stderr may be added in the future in order to support use cases like interactive debugging of a hung test.


Makes sense! I ended up with a slightly different approach than you proposed. Instead of adding an optional pre-timeout field to the Terminating state, I added a new top-level UnitState::PreTimeout state, which itself contains a script_state: UnitState field. This allows the pre-timeout script itself to transition between the standard unit states (running, slow, terminating, exited) with minimal code duplication.

Here's how the output looks in this initial implementation.

Basic statuses:

$ ~/Sites/nextest/target/debug/cargo-nextest nextest run --no-fail-fast info: experimental features enabled: setup-scripts, pre-timeout-scripts Finished `test` profile [unoptimized + debuginfo] target(s) in 0.00s ──────────── Nextest run ID d0f8b05a-5a82-4fa7-9f3c-5cccd3cbccbb with nextest profile: default Starting 2 tests across 1 binary [ 00:00:00] 0/2: SETUP [ 1/1] my-script: ./setup.sh setting up! SETUP PASS [ 0.015s] my-script: ./setup.sh SLOW [> 1.000s] nextest-play tests::it_no_works SLOW [> 1.000s] nextest-play tests::it_works TERMINATING [> 2.000s] nextest-play tests::it_no_works PRETMT nextest-play tests::it_works TIMEOUT [ 2.007s] nextest-play tests::it_no_works ──── STDOUT: nextest-play tests::it_no_works running 1 test PRETMT SLOW [> 1.000s] nextest-play tests::it_works PRETMT PASS [ 1.233s] nextest-play tests::it_works ──── STDOUT: gdb-dump: ./pre-timeout.sh pre-timeout sample stdout ──── STDERR: gdb-dump: ./pre-timeout.sh pre-timeout sample stderr TERMINATING [> 2.000s] nextest-play tests::it_works TIMEOUT [ 3.240s] nextest-play tests::it_works ──── STDOUT: nextest-play tests::it_works running 1 test test sample stdout ──── STDERR: nextest-play tests::it_works test sample stderr ──────────── Summary [ 3.257s] 2 tests run: 0 passed, 2 timed out, 0 skipped TIMEOUT [ 2.007s] nextest-play tests::it_no_works TIMEOUT [ 3.240s] nextest-play tests::it_works error: test run failed

A test with an activated pre-timeout script transitions like so:

Running (not printed)

Slow (printed as SLOW)

Pre-timeout script starting (printed as PRETMT)

Pre-timeout script itself slow (printed as PRETMT SLOW)

Pre-timeout script pass/fail (printed as PRETMT PASS/PRETMT FAIL, followed by the pre-timeout script's stdout/stderr)

Terminating (printed as TERMINATING)

Exit (printed as TIMEOUT, followed by the test's stdout/stderr)

If you request status while the pre-timeout script is executing, you see something like this:

* 1/1: nextest-play tests::it_works status: test running for 2.493s as PID 41610 (marked slow after 1.000s) note: test has reached timeout, pre-timeout script is running: status: script running for 0.485s as PID 41641 stdout: pre-timeout sample stdout stderr: pre-timeout sample stderr

I'm by no means married to any of this. It's just a result of my initial attempt to balance "clear to end users" and "straightforward to implement."

sunshowers · 2025-01-09T20:50:26Z

Thanks! FYI this is on my queue to look at this weekend -- have a lot of stuff at work this week. I like the pretimeout state :)

It's currently ok (I was expecting the impl to be this large), but I think before landing it would be good to get into a series of smaller commits.

benesch · 2025-01-09T20:53:56Z

FYI this is on my queue to look at this weekend -- have a lot of stuff at work this week.

No rush on my end at all—take as long as you need!

I like the pretimeout state :)

🎉 !

It's currently ok (I was expecting the impl to be this large), but I think before landing it would be good to get into a series of smaller commits.

Sounds good! I'm happy to polish up the commit series whenever works best for you.

sunshowers · 2025-01-14T19:37:44Z

Still in the queue to review -- higher priority stuff came up but I'm hoping to carve out some time this weekend.

benesch · 2025-01-14T22:23:42Z

No problem at all, @sunshowers, and thanks for the update!

benesch · 2025-01-21T02:56:16Z

@sunshowers no pressure, but just wanted to check in here!

sunshowers

Think this looks quite good!

sunshowers · 2025-01-22T09:51:32Z

nextest-runner/src/config/scripts.rs

+}
+
+impl PreTimeoutScriptCommand {
+    /// Creates a new `PreTimeoutScriptCommand` for a setup script.


Suggested change

/// Creates a new `PreTimeoutScriptCommand` for a setup script.

/// Creates a new `PreTimeoutScriptCommand` for a pre-timeout script.

Whoops, thanks! Will fix.

sunshowers · 2025-01-22T09:52:10Z

nextest-runner/src/errors.rs

+    #[error("cannot use config script as a {attempted} script: {script}\n(config script is a {actual} script)")]
+    WrongConfigScriptType {


really nice!

sunshowers · 2025-01-22T09:53:14Z

nextest-runner/src/reporter/displayer.rs

+                write!(
+                    writer,
+                    "{}:   test has reached timeout, pre-timeout script is running:",
+                    "note".style(self.styles.count)
+                )?;
+


We have some tests for unit state output -- could you add to them?

For sure. Planning to write a full suite of tests the next moment I have to look at this. Probably this weekend.

sunshowers · 2025-01-22T10:04:11Z

nextest-runner/src/runner/executor.rs

+        // XXX: this function is 95% the same as run_setup_script_inner. Do we
+        // need to try to factor out a shared helper method?


95% the same -- what are the differences? Looking at the function itself, yes -- I'd love to see some common code factored out.

sunshowers · 2025-01-22T10:07:04Z

nextest-runner/src/runner/executor.rs

+    script_id: ScriptId,
+    test: TestPacket<'a>,
+    test_pid: u32,
+    test_stopwatch: &'b StopwatchStart,


would rather avoid adding another lifetime param here to be honest -- can we store a snapshot instead of a reference to the stopwatch?

Yeah, for sure. I didn't love the extra lifetime parameter either.

sunshowers reviewed Dec 26, 2024

View reviewed changes

benesch force-pushed the pre-timeout-scripts branch from ddcc6c7 to 8bd1f1e Compare January 1, 2025 15:58

[squashme] split out separate docs for scripts, address feedback

0e7f441

benesch force-pushed the pre-timeout-scripts branch from 8bd1f1e to 0e7f441 Compare January 1, 2025 16:05

benesch added 2 commits January 1, 2025 12:11

[squashme] specify more of the script lifecycle

aacf019

[squashme] rendering fixes

a107acf

benesch commented Jan 2, 2025

View reviewed changes

benesch added 8 commits January 5, 2025 14:49

[squashme] fix link to setup scripts

4e5a087

[squashme] start plumbing setup scripts

6fa63e6

[squashme] sequentialize execution

b2bc337

[squashme] parse and validate pre-timeout scripts

a9103e6

[squashme] first working implementation

9b68ab5

[squashme] fix existing tests

823eb6e

[squashme] update pre-timeout script env var naming

ed1df40

[squashme] ensure pre-timeout happens before terminating

38dac56

benesch commented Jan 8, 2025

View reviewed changes

[squashme] set special env vars when executing pre-timeout script

121f268

sunshowers reviewed Jan 22, 2025

View reviewed changes


		A given pre-timeout script _S_ is executed when the current profile has at least one rule where the `platform` predicates match the current execution environment, the script _S_ is listed in `pre-timeout`, and a test matching the `filter` has reached its configured timeout.

		Pre-timeout scripts are executed serially, in the order they are defined (_not_ the order they're specified in the rules). If any pre-timeout script exits with a non-zero exit code, an error is logged but the test run continues.

		Nextest will proceed with graceful termination of the test only once the pre-timeout script terminates. See [_How nextest terminates tests_](#defining-pre-timeout-scripts). If the pre-timeout script itself is slow, nextest will apply the same termination protocol to the pre-timeout script.

		The pre-timeout script is not responsible for terminating the test process, but it is permissible for it to do so.


		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:


		Pre-timeout scripts do not support additional configuration options.

		Notably, pre-timeout scripts always capture stdout and stderr. Support for not capturing stdout and stderr may be added in the future in order to support use cases like interactive debugging of a hung test.

	} else if *will_terminate {
	let (required_status_level, style) = if retry_data.is_last_attempt() {
	(StatusLevel::Fail, self.styles.fail)
	} else {
	(StatusLevel::Retry, self.styles.retry)
	};
	if retry_data.total_attempts > 1
	&& self.status_levels.status_level > required_status_level
	{
	write!(
	writer,
	"{:>12} ",
	format!("TRY {} TRMNTG", retry_data.attempt).style(style)
	)?;
	} else {
	write!(writer, "{:>12} ", "TERMINATING".style(style))?;
	};
	}

	/// The current terminating state of a test or script process.
	///
	/// Part of [`UnitState::Terminating`].
	#[derive(Clone, Debug)]
	pub struct UnitTerminatingState {
	/// The process ID.
	pub pid: u32,

	/// The amount of time the unit ran for.
	pub time_taken: Duration,

	/// The reason for the termination.
	pub reason: UnitTerminateReason,

	/// The method by which the process is being terminated.
	pub method: UnitTerminateMethod,

	/// How long has been spent waiting for the process to exit.
	pub waiting_duration: Duration,

	/// How much longer nextest will wait until a kill command is sent to the process.
	pub remaining: Duration,
	}

		// XXX: this function is 95% the same as run_setup_script_inner. Do we
		// need to try to factor out a shared helper method?

	/// Creates a new `PreTimeoutScriptCommand` for a setup script.
	/// Creates a new `PreTimeoutScriptCommand` for a pre-timeout script.

		#[error("cannot use config script as a {attempted} script: {script}\n(config script is a {actual} script)")]
		WrongConfigScriptType {

[DNM] Add documentation for hypothetical pre-timeout scripts #2018

Are you sure you want to change the base?

[DNM] Add documentation for hypothetical pre-timeout scripts #2018

Conversation

benesch commented Dec 26, 2024

codecov bot commented Dec 26, 2024 • edited Loading

Codecov Report

sunshowers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesch commented Dec 29, 2024

benesch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers commented Jan 3, 2025

benesch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers commented Jan 9, 2025 • edited Loading

benesch commented Jan 9, 2025

sunshowers commented Jan 14, 2025

benesch commented Jan 14, 2025

benesch commented Jan 21, 2025

sunshowers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 26, 2024 •

edited

Loading

sunshowers Jan 3, 2025 •

edited

Loading

sunshowers commented Jan 9, 2025 •

edited

Loading