Skip to content

Improve testing #699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
nnethercote merged 3 commits into from
Jul 22, 2020
Merged

Improve testing #699

nnethercote merged 3 commits into from
Jul 22, 2020

Conversation

@nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Jul 21, 2020

With some effort, we can get rid of of bench_test and use bench_local in the test script.

@nnethercote
Copy link
Contributor Author

nnethercote commented Jul 21, 2020

This helps with #681.

Mark-Simulacrum
Mark-Simulacrum approved these changes Jul 21, 2020
View reviewed changes
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This'll need a rebase (but should be relatively simple, this just moved the code around).

I would also like to test CI just in case -- you can probably stick something obviously non-compiling in await-call-tree and see if that fails CI.

Feel free to merge when these are done.

@nnethercote
Inline and remove bench_next.
329ee4a 
It has a single call site.

Also rearrange it a bit, particularly so the error handling is nicer and
more like that of the other subcommands.
@nnethercote
Make all commands return non-zero on benchmark failure.
cd37051 
This will allow commands like `bench_local` to be used in CI testing.

The commit also tweaks how `BenchmarkErrors` works.
@nnethercote
Remove bench_test, add install_next.
35a5e64 
`install_next` + `bench_local` can now be used to do what `bench_test`
previously did. This combination is better because it gives
`bench_local` test coverage.

This required adding a `preserve()` method to `Sysroot`, so that an
installed toolchain could be preserved between `install_next` and
`bench_local`.
@nnethercote
Copy link
Contributor Author

nnethercote commented Jul 21, 2020

I did a local run with a deliberately-failing await-call-tree. It's a good idea to test that case on CI as well.

@nnethercote
Copy link
Contributor Author

nnethercote commented Jul 21, 2020

"Test benchmarks" failed as expected with the deliberate error.

Here is the output showing await-call-tree failing:

37 benchmarks remaining
Preparing await-call-tree (with Check)
[2020-07-21T22:02:46Z DEBUG collector::execute] "/home/runner/work/rustc-perf/rustc-perf/cache/8ad7bc3f428300aee6764f6e23527e19eb235e81/bin/cargo" "rustc" "--manifest-path" "Cargo.toml" "-p" "file:///tmp/.tmpYw1hCR#await-call-tree:0.1.0" "--profile" "check" "--"
collector error: Failed to benchmark 'await-call-tree', recorded: expected success, got exit code: 101

stderr=    Checking await-call-tree v0.1.0 (/tmp/.tmpYw1hCR)
error: expected one of `!` or `::`, found `macro_rules`
 --> src/lib.rs:6:1
  |
4 | blah
  |     - expected one of `!` or `::`
5 | 
6 | macro_rules! mk_async_fn {
  | ^^^^^^^^^^^ unexpected token

error: aborting due to previous error

And here is the output showing that the test failed overall as a result:

collection took 2087.964352436s
collector error: 1 benchmarks failed
   0: anyhow::private::new_adhoc
             at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/anyhow-1.0.31/src/backtrace.rs:10
   1: collector::BenchmarkErrors::fail_if_nonzero
             at ./collector/src/main.rs:183
   2: collector::main_result
             at ./collector/src/main.rs:565
   3: collector::main
             at ./collector/src/main.rs:413
   4: std::rt::lang_start::{{closure}}
             at /rustc/f9a3086363f214f2b56bef30f0ac572e1a9127f1/src/libstd/rt.rs:67
   5: std::rt::lang_start_internal::{{closure}}
             at /rustc/f9a3086363f214f2b56bef30f0ac572e1a9127f1/src/libstd/rt.rs:52
      std::panicking::try::do_call
             at /rustc/f9a3086363f214f2b56bef30f0ac572e1a9127f1/src/libstd/panicking.rs:348
      std::panicking::try
             at /rustc/f9a3086363f214f2b56bef30f0ac572e1a9127f1/src/libstd/panicking.rs:325
      std::panic::catch_unwind
             at /rustc/f9a3086363f214f2b56bef30f0ac572e1a9127f1/src/libstd/panic.rs:394
      std::rt::lang_start_internal
             at /rustc/f9a3086363f214f2b56bef30f0ac572e1a9127f1/src/libstd/rt.rs:51
   6: std::rt::lang_start
             at /rustc/f9a3086363f214f2b56bef30f0ac572e1a9127f1/src/libstd/rt.rs:67
   7: main
   8: __libc_start_main
   9: _start

+ code=1
+ kill 9967
+ exit 1
##[error]Process completed with exit code 1.

@nnethercote
Copy link
Contributor Author

nnethercote commented Jul 21, 2020

I have repushed with the deliberate error removed.

@nnethercote nnethercote merged commit 48cdb26 into rust-lang:master Jul 22, 2020
@nnethercote nnethercote deleted the improve-testing branch July 22, 2020 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nnethercote @Mark-Simulacrum