Skip to content
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.

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

libtest: add --show-output flag to print stdout of successful tests #62600

Merged
merged 2 commits into from
Aug 27, 2019

Conversation

emmericp
Copy link
Contributor

This pull request adds a new flag --show-output for tests to show the output of successful tests. For most formatters this was already supported just not exposed via the CLI (apparently only used by librustdoc). I've also added support for this option in the JSON formatter.

This kind of fixes #54669 which wants --format json to work with --nocapture, which is... well, impossible. What this issue really calls for is --show-output as implemented here.

this new flag enables printing the captured stdout of successful tests
utilizing the already existing display_output test runner option
@Centril Centril added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. labels Jul 11, 2019
@Centril Centril added this to the 1.38 milestone Jul 11, 2019
@Centril
Copy link
Contributor

Centril commented Jul 11, 2019

Assigning random T-compiler member...

r? @varkor

@rust-highfive rust-highfive assigned varkor and unassigned bluss Jul 11, 2019
@emmericp
Copy link
Contributor Author

updated with a new test case for the json output

@varkor
Copy link
Member

varkor commented Jul 22, 2019

Assigning someone more familiar with libtest JSON output as in #46450.

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned varkor Jul 22, 2019
@jonas-schievink jonas-schievink added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 26, 2019
@wirelessringo
Copy link

Ping from triage. @nrc any updates on this? Thank you.

@JohnTitor
Copy link
Member

Ping from triage: @nrc do you have time to review?

@Centril Centril modified the milestones: 1.38, 1.39 Aug 13, 2019
@Centril
Copy link
Contributor

Centril commented Aug 20, 2019

cc @gnzlbg

@@ -272,7 +272,7 @@ impl Options {

// The default console test runner. It accepts the command line
// arguments and a vector of test_descs.
pub fn test_main(args: &[String], tests: Vec<TestDescAndFn>, options: Options) {
pub fn test_main(args: &[String], tests: Vec<TestDescAndFn>, options: Option<Options>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really sure why this change is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The two places that call this function internally are:

  • librustdoc, which gets the display_output options from some config somewhere else, so we don't want to override it from the commandline parameter here (at least not flag not present overring the config), so can't override display_output
  • test_main_static, which wants to get display_output from the not yet parsed args list, so we need to set it here from the command line

I guess it would be possible to change librustdoc to add/remove the --show-output flag in the args array when its internal config option is present/not present, but i think having options as an Option to override this is nicer than fiddling with cli args.

So the actual problem is that there are basically two structs storing test options, it's overall messy. I didn't want to make a big change to that logic here

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. I just took a look and unifying these isn't trivial.

Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

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

The json formatting part LGTM. Otherwise I only left a question.

@Centril
Copy link
Contributor

Centril commented Aug 21, 2019

r? @gnzlbg

cc @rust-lang/compiler @rust-lang/dev-tools for FCP.

@Centril
Copy link
Contributor

Centril commented Aug 21, 2019

@bors delegate=gnzlbg

@bors
Copy link
Contributor

bors commented Aug 21, 2019

✌️ @gnzlbg can now approve this pull request

@emmericp
Copy link
Contributor Author

thanks for your review!

@estebank
Copy link
Contributor

@bors r=gnzlbg

@bors
Copy link
Contributor

bors commented Aug 26, 2019

📌 Commit 409a41d has been approved by gnzlbg

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 26, 2019
@bors
Copy link
Contributor

bors commented Aug 27, 2019

⌛ Testing commit 409a41d with merge 276ad3ce0cc3777c2aeb6b34f12a809a4f950fe6...

Centril added a commit to Centril/rust that referenced this pull request Aug 27, 2019
…=gnzlbg

libtest: add --show-output flag to print stdout of successful tests

This pull request adds a new flag `--show-output` for tests to show the output of successful tests. For most formatters this was already supported just not exposed via the CLI (apparently only used by `librustdoc`). I've also added support for this option in the JSON formatter.

This kind of fixes rust-lang#54669 which wants `--format json` to work with `--nocapture`, which is... well, impossible. What this issue really calls for is `--show-output` as implemented here.
@Centril
Copy link
Contributor

Centril commented Aug 27, 2019

@bors retry rolled up.

@bors
Copy link
Contributor

bors commented Aug 27, 2019

⌛ Testing commit 409a41d with merge e8bed89a0bb6019c970e3f22c91eb6652254801f...

Centril added a commit to Centril/rust that referenced this pull request Aug 27, 2019
…=gnzlbg

libtest: add --show-output flag to print stdout of successful tests

This pull request adds a new flag `--show-output` for tests to show the output of successful tests. For most formatters this was already supported just not exposed via the CLI (apparently only used by `librustdoc`). I've also added support for this option in the JSON formatter.

This kind of fixes rust-lang#54669 which wants `--format json` to work with `--nocapture`, which is... well, impossible. What this issue really calls for is `--show-output` as implemented here.
@Centril
Copy link
Contributor

Centril commented Aug 27, 2019

@bors retry rolled up.

bors added a commit that referenced this pull request Aug 27, 2019
Rollup of 4 pull requests

Successful merges:

 - #62600 (libtest: add --show-output flag to print stdout of successful tests)
 - #63698 (Fixed floating point issue with asinh function)
 - #63761 (Propagate spans and attributes from proc macro definitions)
 - #63917 (Error when generator trait is not found)

Failed merges:

r? @ghost
@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 27, 2019

I thought this was awaiting some sort of FCP since the option isn't an unstable one (cc @rust-lang/dev-tools ?). This option feels harmless, and we still have a nightly cycle to make it unstable if we want to, so I'm fine with merging this as is.

@bors bors merged commit 409a41d into rust-lang:master Aug 27, 2019
bors bot added a commit to intellij-rust/intellij-rust that referenced this pull request Aug 30, 2019
4139: RUN: enable --show-output when executing tests r=mchernyavsky a=emmericp

Fixes #3994

This requires this patch for rust (hopefully in 1.38?): rust-lang/rust#62600

This is work in progress because it doesn't work yet, I'm looking for some help here:
The problem is you can't run `rustVersion()` in the UI thread and I'd really like to have this behavior as default if the compiler is new enough.
What's the best way to achieve this? Fallback would be to make this an option for the run configuration, but that would be a little bit annoying since virtually everyone would want to enable this?


Edit: changed the code to hardcode 1.38 as Rust version passed to `patchArgs` because I'm using this branch ;) Looking for a good way to get the Rust version here.

<!--
Hello and thank you for the pull request!

We don't have any strict rules about pull requests, but you might check
https://github.com/intellij-rust/intellij-rust/blob/master/CONTRIBUTING.md
for some hints!

Note that we need an electronic CLA for contributions:
https://github.com/intellij-rust/intellij-rust/blob/master/CONTRIBUTING.md#cla

After you sign the CLA, please add your name to
https://github.com/intellij-rust/intellij-rust/blob/master/CONTRIBUTORS.txt

:)
-->


Co-authored-by: Paul Emmerich <paul.emmerich@croit.io>
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 11, 2019
Pkgsrc changes:
 * Remove patch which no longer applies (but what about RPATH?)
 * Adapt a few patches to changed files upstream.

Upstream changes:

Version 1.39.0 (2019-11-07)
===========================

Language
--------
- [You can now create `async` functions and blocks with `async fn`,
  `async move {}`, and `async {}` respectively, and you can now call
  `.await` on async expressions.][63209]
- [You can now use certain attributes on function, closure, and function
  pointer parameters.][64010] These attributes include `cfg`, `cfg_attr`,
  `allow`, `warn`, `deny`, `forbid` as well as inert helper attributes used
  by procedural macro attributes applied to items. e.g.
  ```rust
  fn len(
      #[cfg(windows)] slice: &[u16],
      #[cfg(not(windows))] slice: &[u8],
  ) -> usize {
      slice.len()
  }
  ```
- [You can now take shared references to bind-by-move patterns in the
  `if` guards of `match` arms.][63118] e.g.
  ```rust
  fn main() {
      let array: Box<[u8; 4]> = Box::new([1, 2, 3, 4]);

      match array {
          nums
  //      ---- `nums` is bound by move.
              if nums.iter().sum::<u8>() == 10
  //                 ^------ `.iter()` implicitly takes a reference to `nums`.
          => {
              drop(nums);
  //          ----------- Legal as `nums` was bound by move and so we have ownership.
          }
          _ => unreachable!(),
      }
  }
  ```

Compiler
--------
- [Added tier 3\* support for the `i686-unknown-uefi` target.][64334]
- [Added tier 3 support for the `sparc64-unknown-openbsd` target.][63595]
- [rustc will now trim code snippets in diagnostics to fit in your terminal.]
  [63402] **Note** Cargo currently doesn't use this feature. Refer to
  [cargo#7315][cargo/7315] to track this feature's progress.
- [You can now pass `--show-output` argument to test binaries to print the
  output of successful tests.][62600]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
---------
- [`Vec::new` and `String::new` are now `const` functions.][64028]
- [`LinkedList::new` is now a `const` function.][63684]
- [`str::len`, `[T]::len` and `str::as_bytes` are now `const` functions.][63770]
- [The `abs`, `wrapping_abs`, and `overflowing_abs` numeric functions are
  now `const`.][63786]

Stabilized APIs
---------------
- [`Pin::into_inner`]
- [`Instant::checked_duration_since`]
- [`Instant::saturating_duration_since`]

Cargo
-----
- [You can now publish git dependencies if supplied with a `version`.]
  [cargo/7237]
- [The `--all` flag has been renamed to `--workspace`.][cargo/7241] Using
  `--all` is now deprecated.

Misc
----
- [You can now pass `-Clinker` to rustdoc to control the linker used
  for compiling doctests.][63834]

Compatibility Notes
-------------------
- [Code that was previously accepted by the old borrow checker, but rejected by
  the NLL borrow checker is now a hard error in Rust 2018.][63565] This was
  previously a warning, and will also become a hard error in the Rust 2015
  edition in the 1.40.0 release.
- [`rustdoc` now requires `rustc` to be installed and in the same directory to
  run tests.][63827] This should improve performance when running a large
  amount of doctests.
- [The `try!` macro will now issue a deprecation warning.][62672] It is
  recommended to use the `?` operator instead.
- [`asinh(-0.0)` now correctly returns `-0.0`.][63698] Previously this
  returned `0.0`.

[62600]: rust-lang/rust#62600
[62672]: rust-lang/rust#62672
[63118]: rust-lang/rust#63118
[63209]: rust-lang/rust#63209
[63402]: rust-lang/rust#63402
[63565]: rust-lang/rust#63565
[63595]: rust-lang/rust#63595
[63684]: rust-lang/rust#63684
[63698]: rust-lang/rust#63698
[63770]: rust-lang/rust#63770
[63786]: rust-lang/rust#63786
[63827]: rust-lang/rust#63827
[63834]: rust-lang/rust#63834
[63927]: rust-lang/rust#63927
[63933]: rust-lang/rust#63933
[63934]: rust-lang/rust#63934
[63938]: rust-lang/rust#63938
[63940]: rust-lang/rust#63940
[63941]: rust-lang/rust#63941
[63945]: rust-lang/rust#63945
[64010]: rust-lang/rust#64010
[64028]: rust-lang/rust#64028
[64334]: rust-lang/rust#64334
[cargo/7237]: rust-lang/cargo#7237
[cargo/7241]: rust-lang/cargo#7241
[cargo/7315]: rust-lang/cargo#7315
[`Pin::into_inner`]: https://doc.rust-lang.org/std/pin/struct.Pin.html#method.into_inner
[`Instant::checked_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.checked_duration_since
[`Instant::saturating_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.saturating_duration_since
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using --format=json with --nocapture