Skip to content

Add libtest json output option#1086

Merged
sunshowers merged 6 commits into
nextest-rs:mainfrom
EmbarkStudios:libtest-output
Dec 10, 2023
Merged

Add libtest json output option#1086
sunshowers merged 6 commits into
nextest-rs:mainfrom
EmbarkStudios:libtest-output

Conversation

@Jake-Shadle

Copy link
Copy Markdown
Contributor

There are a couple of todos, but the biggest missing piece is combined stdout/err output.

Opening this as a draft while I make the combined output helper.

There are a couple of todos, but the biggest missing piece is combined
stdout/err output
@Jake-Shadle Jake-Shadle changed the title Checkpoint initial implementation Add libtest json output option Nov 1, 2023
@codecov

codecov Bot commented Nov 1, 2023

Copy link
Copy Markdown

Codecov Report

Attention: 330 lines in your changes are missing coverage. Please review.

Comparison is base (b7c376e) 79.13% compared to head (54618a0) 78.00%.
Report is 91 commits behind head on main.

Files Patch % Lines
nextest-runner/src/reporter/structured/libtest.rs 0.00% 321 Missing ⚠️
cargo-nextest/src/errors.rs 0.00% 4 Missing ⚠️
nextest-runner/src/reporter/structured.rs 57.14% 3 Missing ⚠️
cargo-nextest/src/dispatch.rs 86.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1086      +/-   ##
==========================================
- Coverage   79.13%   78.00%   -1.13%     
==========================================
  Files          64       69       +5     
  Lines       16555    17329     +774     
==========================================
+ Hits        13100    13518     +418     
- Misses       3455     3811     +356     

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

@sunshowers

sunshowers commented Nov 1, 2023

Copy link
Copy Markdown
Member

Thanks, will have a look at it over the weekend.

For stdout/stderr, can we just do something like:

*** STDOUT ***
<stdout>
*** STDERR ***
<stderr>

It's not great but it is emulation, after all. Seems like trying to maintain a combined buffer (or separate buffers with indexes) is possible but also adds some nontrivial complexity.

@sunshowers

Copy link
Copy Markdown
Member

Just FYI I've had too may commitments to review this, but hope to get to it this weekend.

@Jake-Shadle

Copy link
Copy Markdown
Contributor Author

No worries, I'm doing a bunch of other stuff atm.

@sunshowers sunshowers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for your patience. This looks pretty good overall. Let's get this in before working on the combined output.

Comment thread nextest-runner/src/reporter/structured.rs Outdated
Comment thread nextest-runner/src/reporter/structured/libtest.rs Outdated
Comment thread nextest-runner/src/reporter/structured/libtest.rs Outdated
Comment thread nextest-runner/src/reporter/structured/libtest.rs
Comment thread nextest-runner/src/reporter/structured/libtest.rs Outdated
Comment thread nextest-runner/src/reporter/structured/libtest.rs
Comment thread nextest-runner/src/reporter/structured/libtest.rs
Running was the _total_ number of running tests, not just the ones in
the same binary, which would result
in only one binary output being written
@Jake-Shadle Jake-Shadle marked this pull request as ready for review November 29, 2023 07:42
@sunshowers

Copy link
Copy Markdown
Member

Thanks for this. I really apologize for how long this has taken, I've been dealing with serious personal life issues.

I think this is good to go. The only question I have is whether it's ok for this to be experimental for at least a short while. You'd have to add experimental = ["libtest-json"] to .config/nextest.toml, similar to setup scripts. (I'm happy to implement this.)

@Jake-Shadle

Copy link
Copy Markdown
Contributor Author

I could do that, but I don't understand the need for that additional hurdle when it already requires passing a new flag?

@sunshowers

sunshowers commented Dec 10, 2023

Copy link
Copy Markdown
Member

It's true that it requires passing a new flag, but as machine-readable output this would need to be a stable format (which experimental features are not). I think there's just a few more things we need to do before I'd feel comfortable stabilizing this:

  • Use the format in our integration tests to shake out issues
  • Document the format
  • Get some real-world use to ensure that it's serving users' needs

Once this is stabilized the format will have to be maintained forever (per our stability policy), so it's worth spending some time making sure it's all good.

I'll land this and make it experimental, before getting a release out tomorrow or so. Thanks for all your work!

@sunshowers sunshowers merged commit 2728de2 into nextest-rs:main Dec 10, 2023
@Jake-Shadle Jake-Shadle deleted the libtest-output branch December 11, 2023 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants