feat: treat timeouts as failure or success based on config

[eduardorittner]

Adds a new configuration option inside "slow-timeout": "on-timeout" which when set to "pass" makes nextest treat timed out tests as passing.

This PR adds a new SlowTimeoutResult struct similar to LeakTimeoutResult which characterizes a timeout as passing or failing.

There are some things which need to be addressed, and most of them are marked with TODOs in the code, these should all be fixed before merging. Most of them have to do with how to display this information, since we now have a new category of tests to display: tests that passed with a timeout. Any opinions here would be great!
There are some failing tests due to the formatting changes made, I decided not to accept the snapshots just yet since I'm not sure this is the final formatting we're going to end up with.

P.S.: I noticed nextest follows atomic commits, I figured only one commit would be enough for this change but I can split it up into smaller commits if it's deemed better.

Closes #1291

[eduardorittner]

Just confirmed that the failing tests in CI are these 3, which are snapshot tests for the formatting output. So all other tests are passing

nextest-runner reporter::displayer::imp::tests::test_info_response
nextest-runner reporter::displayer::imp::tests::test_summary_line
nextest-runner reporter::displayer::progress::tests::progress_str_snapshots

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.20%. Comparing base (8832117) to head (5308edd).
⚠️ Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
nextest-runner/src/reporter/displayer/imp.rs	64.28%	5 Missing ⚠️
nextest-runner/src/reporter/events.rs	75.00%	5 Missing ⚠️
nextest-runner/src/reporter/structured/libtest.rs	0.00%	2 Missing ⚠️
nextest-runner/src/reporter/aggregator/junit.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2742      +/-   ##
==========================================
+ Coverage   80.12%   80.20%   +0.07%     
==========================================
  Files         113      113              
  Lines       26167    26356     +189     
==========================================
+ Hits        20967    21138     +171     
- Misses       5200     5218      +18     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sunshowers]

Thanks. One commit is fine for this change (though there's one particular thing in this PR that should be in a followup).

[sunshowers]

An inconsistency here -- we use SlowTimeoutResult::Fail here but default below. Let's pick one or the other -- I'm partial to using Fail and removing the Default impl, unless there's a good reason to retain it.

[eduardorittner]

The only issue with not having a SlowTimeoutResult::default is that AFAICT (I'm not that familiar with serde) the #[serde(default)] line in SlowTimeout's definition will have to be something like #[serde(default = "default_result")] where default_result is a custom function that returns the default value for SlowTimeoutResult. At that point I think it's just better to just keep the derived default and change this usage from SlowTimeoutResult::Fail to default.

From

pub struct SlowTimeout {
    // Omitted fields
    #[serde(default)]
    pub(crate) on_timeout: SlowTimeoutResult,
}

To

pub struct SlowTimeout {
    // Omitted fields
    #[serde(default = "default_on_timeout")]
    pub(crate) on_timeout: SlowTimeoutResult,
}

[eduardorittner]

I just noticed something, since VERY_LARGE is const, I can't actually call SlowTimeoutResult::default() since it's not const, so now I'm unsure what to do:

1. Keep #[derive(Default)] in SlowTimeoutResult, use SlowTimeoutResult::default in non-const contexts and SlowTimeoutResult::Fail in const contexts.
2. Keep #[derive(Default)] just for #[serde(default)], use SlowTimeoutResult::Fail everywhere else
3. Remove #[derive(Default)], add a pub const fn default() -> Self which can be used everywhere. This does mean that instead of #[serde(default)] we'll have #[serde(default = "SlowTimeoutResult::default")] in SlowTimeout's definition.

Option 1 doesn't seem that great to me, I think I prefer option 3, but option 2 is also fine.

[eduardorittner]

So I've split off the changes into two commits, where the second one only deals with the logic in reporter/events.rs for counting the number of failed tests, timed out tests, etc. Some tests are now also failing because I made self.failed include self.failed_timed_out, instead of them being disjoint, which broke a bunch of the tests that relied on this logic. I can revert this and make them disjoint again if that's better

[eduardorittner]

Tests are now passing, I changed the logic in events.rs so that self.failed and self.failed_timed_out are disjoint and self.failed_count() is now

pub fn failed_count(&self) -> usize {
    self.failed + self.exec_failed + self.failed_timed_out
}

I haven't added any new tests, but I'll get started on that now that all the old existing tests are passing.

[sunshowers]

Looks great -- yeah, just missing new tests otherwise this is good to go.

[eduardorittner]

I have two questions regarding junit:

1. Should passing timed out tests be reported as failures? I ask because in reporter/aggregator/junit.rs passing tests that leaked are reported as NonSuccessKind::Error: https://github.com/eduardorittner/nextest/blob/42f92f1ed6c33eba1965f93bdb862548764892b3/nextest-runner/src/reporter/aggregator/junit.rs#L353-L358

2. How/where is junit support tested?

[sunshowers]

1. Good question. I think it should be marked as passed.
2. Uhmmm, there aren't good tests for deserializing JUnit reports. But there probably should be. I wouldn't worry about that, at some point I'll have Claude Code or something write those tests.

[sunshowers]

Uhmmm, there aren't good tests for deserializing JUnit reports. But there probably should be. I wouldn't worry about that, at some point I'll have Claude Code or something write those tests.

Did this in #2761.

[eduardorittner]

Hi! Something I just thought of, how should retries be handled when on-timeout="pass" is configured? Right now what happens is that timed out tests are not retried, since they were marked as passing the first time around. This behavior makes sense to me (the alternative doesn't seem that great, to retry "passing" tests until the maximum number of retries just to mark them as passing again), but is maybe not that intuitive? Maybe issue a warning when both on-timeout="pass" and retries are configured?

[sunshowers]

@eduardorittner I think not retrying is correct -- I also don't think a warning is necessary here, because the rules are very simple (pass = no retry, fail for any reason = retry).

[eduardorittner]

Added some new tests but they're now failing on windows, will try to fix that once I have some more time

[sunshowers]

I'm probably missing something but it looks like some of the comments you marked resolved aren't actually resolved yet -- are you planning to work on them?

[eduardorittner]

yeah I'm sorry, hadn't even noticed that. I was working from two different computers and without realising it lost some of the changes I made whoch addressed them. I'll go back through every one to fix them again. My bad!

[eduardorittner]

I've now gone over the resolved comments and I think I've re-resolved them all!