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

Add libtest json output option #1086

Merged
merged 6 commits into from
Dec 10, 2023

Conversation

Jake-Shadle
Copy link
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
Copy link

codecov bot commented Nov 1, 2023

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
Copy link
Member

sunshowers commented Nov 1, 2023

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
Member

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

@Jake-Shadle
Copy link
Contributor Author

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

Copy link
Member

@sunshowers sunshowers left a comment

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.

nextest-runner/src/reporter/structured.rs Outdated Show resolved Hide resolved
nextest-runner/src/reporter/structured/libtest.rs Outdated Show resolved Hide resolved
nextest-runner/src/reporter/structured/libtest.rs Outdated Show resolved Hide resolved
nextest-runner/src/reporter/structured/libtest.rs Outdated Show resolved Hide resolved
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
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
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
Copy link
Member

sunshowers commented Dec 10, 2023

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
13 checks passed
@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