Add warning timeout for tests that run >1min #35405

Merged
merged 2 commits into from Aug 10, 2016

Conversation

Projects
None yet
6 participants
@futile

futile commented Aug 6, 2016

This makes it easier to identify hanging tests. As described in #2873,
when a test doesn't finish, we so far had no information on which test
that was. In this PR, we add a duration of 60 seconds for each test,
after which a warning will be printed mentioning that this specific test
has been running for a long time already.

Example output:
https://gist.github.com/futile/6ea3eed85fe632df8633c1b03c08b012

r? @brson

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Aug 6, 2016

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Collaborator

rust-highfive commented Aug 6, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@brson brson added the relnotes label Aug 6, 2016

src/libtest/lib.rs
+
+ for rem in to_remove {
+ running_tests.remove(&rem);
+ }

This comment has been minimized.

@brson

brson Aug 6, 2016

Contributor

Can the book-keeping here be extracted to a function so it doesn't obscure the main logic of the loop?

@brson

brson Aug 6, 2016

Contributor

Can the book-keeping here be extracted to a function so it doesn't obscure the main logic of the loop?

This comment has been minimized.

@brson

brson Aug 6, 2016

Contributor

That is, everything from line 905 to here.

@brson

brson Aug 6, 2016

Contributor

That is, everything from line 905 to here.

src/libtest/lib.rs
+ for (desc, time_left) in &mut running_tests {
+ if *time_left >= elapsed {
+ *time_left -= elapsed;
+ } else {

This comment has been minimized.

@brson

brson Aug 6, 2016

Contributor

You may be able to avoid doing these updates if instead of storing the remaining Duration in the hash map you instead stored an Instant representing the deadline. If that makes sense, do you mind trying to reformulate it like that and seeing if it results in less code?

@brson

brson Aug 6, 2016

Contributor

You may be able to avoid doing these updates if instead of storing the remaining Duration in the hash map you instead stored an Instant representing the deadline. If that makes sense, do you mind trying to reformulate it like that and seeing if it results in less code?

This comment has been minimized.

@futile

futile Aug 6, 2016

That's a good idea, I'll try that out!

@futile

futile Aug 6, 2016

That's a good idea, I'll try that out!

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Aug 6, 2016

Contributor

Thanks @futile! Looks great.

Contributor

brson commented Aug 6, 2016

Thanks @futile! Looks great.

Felix Rath
add warning timeout for tests that run >1min
this makes it easier to identify hanging tests
@futile

This comment has been minimized.

Show comment
Hide comment
@futile

futile Aug 6, 2016

updated the PR and addressed the comments.

futile commented Aug 6, 2016

updated the PR and addressed the comments.

Felix Rath
save an Instant for the timeout instead of a Duration
requires less bookkeeping. also move some functionality into functions,
to keep the loop cleaner.
@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Aug 8, 2016

Contributor

cc @rust-lang/tools this is adding warning message whenever a test runs for over 60 seconds.

Contributor

brson commented Aug 8, 2016

cc @rust-lang/tools this is adding warning message whenever a test runs for over 60 seconds.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Aug 8, 2016

Contributor

@bors r+

Contributor

brson commented Aug 8, 2016

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 8, 2016

Contributor

📌 Commit e995061 has been approved by brson

Contributor

bors commented Aug 8, 2016

📌 Commit e995061 has been approved by brson

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 9, 2016

Contributor

⌛️ Testing commit e995061 with merge 7dfc03a...

Contributor

bors commented Aug 9, 2016

⌛️ Testing commit e995061 with merge 7dfc03a...

bors added a commit that referenced this pull request Aug 9, 2016

Auto merge of #35405 - futile:tests_warn_timeout, r=brson
Add warning timeout for tests that run >1min

This makes it easier to identify hanging tests. As described in #2873,
when a test doesn't finish, we so far had no information on which test
that was. In this PR, we add a duration of 60 seconds for each test,
after which a warning will be printed mentioning that this specific test
has been running for a long time already.

Example output:
https://gist.github.com/futile/6ea3eed85fe632df8633c1b03c08b012

r? @brson
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 9, 2016

Contributor

💔 Test failed - auto-win-msvc-64-opt-rustbuild

Contributor

bors commented Aug 9, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Aug 9, 2016

Member

@bors: retry

On Tuesday, August 9, 2016, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt-rustbuild/builds/2121


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#35405 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95GNhY5VEGXY3HL1hee1Y5_OR24N6ks5qeERYgaJpZM4JeJ3M
.

Member

alexcrichton commented Aug 9, 2016

@bors: retry

On Tuesday, August 9, 2016, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt-rustbuild/builds/2121


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#35405 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95GNhY5VEGXY3HL1hee1Y5_OR24N6ks5qeERYgaJpZM4JeJ3M
.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 10, 2016

Contributor

⌛️ Testing commit e995061 with merge ae77410...

Contributor

bors commented Aug 10, 2016

⌛️ Testing commit e995061 with merge ae77410...

bors added a commit that referenced this pull request Aug 10, 2016

Auto merge of #35405 - futile:tests_warn_timeout, r=brson
Add warning timeout for tests that run >1min

This makes it easier to identify hanging tests. As described in #2873,
when a test doesn't finish, we so far had no information on which test
that was. In this PR, we add a duration of 60 seconds for each test,
after which a warning will be printed mentioning that this specific test
has been running for a long time already.

Example output:
https://gist.github.com/futile/6ea3eed85fe632df8633c1b03c08b012

r? @brson

@bors bors merged commit e995061 into rust-lang:master Aug 10, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@Wojtek242

This comment has been minimized.

Show comment
Hide comment
@Wojtek242

Wojtek242 Jul 3, 2018

Is it possible to disable this warning or increase the time out? I am running some integration tests that do some reasonably intensive calculations that take a few minutes to complete and the 60 second warning is misleading in this case.

Is it possible to disable this warning or increase the time out? I am running some integration tests that do some reasonably intensive calculations that take a few minutes to complete and the 60 second warning is misleading in this case.

@futile

This comment has been minimized.

Show comment
Hide comment
@futile

futile Jul 3, 2018

@Wojtek242 this seems useful, and was actually also discussed in #2873 before. There the consensus was that this functionality should be its own issue. Personally, I think it wouldn't be too much effort to add the required functionality to rustc. The code and files changed in this PR should be a good start!

futile commented Jul 3, 2018

@Wojtek242 this seems useful, and was actually also discussed in #2873 before. There the consensus was that this functionality should be its own issue. Personally, I think it wouldn't be too much effort to add the required functionality to rustc. The code and files changed in this PR should be a good start!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment