#107307 Adding additional information to "cargo test --list" output #107785

parthopdas · 2023-02-08T02:42:50Z

earlier:

now:

// test | test_type | ignored_or_active | location_info

… output earlier: p2dchecks::fibonacci_test::case_1: test now: // test test_type | ignored_or_not | location_info p2dchecks::fibonacci_test::case_1: test | false | src\lib.rs:57:8: 57:19

rustbot · 2023-02-08T02:42:56Z

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

Please see the contribution instructions for more information.

rustbot · 2023-02-08T02:42:59Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

… output fixing build break

… output attempting to fix ordering issue

… output wtf am i doing?

… output wtf am i doing (redux)?

… output fixing tests

library/test/src/tests.rs

compiler/rustc_span/src/source_map.rs

petrochenkov · 2023-02-08T10:12:04Z

.gitignore

@@ -21,6 +21,7 @@ Session.vim
 .project
 .favorites.json
 .settings/
+.vs/


Could you avoid changes to this file?

this is the visual studio ide creating the file. like .vscode.

with the extension i have just released (rust-analy - for which this PR is, i do expect windows devs to use visual studio more often.

is it okay to retain it?

compiler/rustc_builtin_macros/src/test.rs

library/test/src/console.rs

petrochenkov · 2023-02-08T10:26:58Z

Not sure who is responsible for libtest interfaces like this.
Marked this as waiting on @rust-lang/devtools (it seems like you don't have a dedicated I-*-nominated label).

cc @matklad as well, since you may know people who knows something about vscode integration.

matklad · 2023-02-08T11:53:01Z

cc @Veykril

Some high-level non-normative comments:

Ideally, IDEs would get this info from static analysis of the code, rather than from compiling and running the binary
Not everything is an advanced IDE, so having this info available helps low-tech scenarios a lot and, in principle, we should have it
The current output is de-facto stable, so we can't change it without some extra opt-in flag
More generally, the standard solution in this space is to emit the info in JSON format, to get us leeway to add new information later in a backwards compatible way
So we probably want something a-la --list-tests --message-format json
Similarly, I don't know who is the right reviewer here

parthopdas · 2023-02-08T13:46:21Z

@matklad @Veykril

thanks for you comments - they all make sense.

The current output is de-facto stable, so we can't change it without some extra opt-in flag

i did my best to make this back-compatible - but if you let me know how to add an opt in flag, i am happy to do that.

More generally, the standard solution in this space is to emit the info in JSON format, to get us leeway to add new information later in a backwards compatible way

So we probably want something a-la --list-tests --message-format json

the issue with json flag - this is my observation and i could be wrong - is that they are only available in the nightly builds. we cannot expect tool users to use only the nightly builds.

… output taking in PR comments.

… output fixing build break.

Manishearth · 2023-02-08T14:44:02Z

Not the devtools team, libtest output is libs or compiler.

(probably libs)

Manishearth · 2023-02-08T14:47:19Z

My general take here is that this would require a lot more discussion before landing and probably should not be done as a PR.

And also I think the proper solution is fully machine readable output for cargo test, which requires an RFC.

… output making the delimiter consistent.

parthopdas · 2023-02-08T15:06:03Z

@Manishearth some clarifying questions.

My general take here is that this would require a lot more discussion before landing and probably should not be done as a PR.

are you referring to "--message-format json"? if so i agree. i am happy to start one and then raise the PR.

however if you are referring to this entire change, it will be good to understand your reasoning.

my reasoning is that:

navigating from test explorer back to code is an important scenario when an engineer has 100s of tests - and this is not possible today. this is typically the case with even simpler of my projects. take e.g. rust-analyzer.v for which this change is required and which enables rust development on visual studio ide.
i am just extending the current format : with additional fields.
so i feel we should not be holding this up.

And also I think the proper solution is fully machine readable output for cargo test, which requires an RFC.

i agree and happy to start it.

ehuss · 2023-02-08T19:35:09Z

the issue with json flag - this is my observation and i could be wrong - is that they are only available in the nightly builds. we cannot expect tool users to use only the nightly builds.

If you are referring to the --format=json flag, it can be used on stable with the -Zunstable-options flag. There is a hole in the unstable restrictions for libtest, but not one anyone has been motivated to close.

We can also stabilize --format=json when used in conjunction with --list without stabilizing the JSON output as a whole. (It's a bit unfortunate that every tool is using a different flag here, but I think it is not worth trying to address that.) I don't want to speak for the libs team, but I would expect that to be something that is easier to do.

I agree with @Manishearth that there probably should be more discussion. I assume there aren't already standard formats for just listing tests (such as junit)? I would starting a thread on internals to see if anyone in a wider audience has any feedback for adding a JSON format for listing tests, along with a proposed structure. I would also write a comment on #49359 soliciting feedback (since some people following that probably have some familiarity with machine-readable test integration).

parthopdas · 2023-02-08T19:59:04Z

@ehuss

few things to set the context

typically test runners like in vscode and soon in visual studio work in 2 phases - test discovery then test execution. test discovery lists tests on a test explorer from which the dev can then run / debug tests.
i did explore the static analysis option that rust-analyzer on vscode uses to populate the code lens. however this is a no-go for me as visual studio does not do the same and would require a fair bit of effort for me to implement at the extension level + it would degrade user experience (2 x RLS per IDE) to implement.

point#1 means we cannot run the test and then populate the test explorer experience -> that beats the point of test explorer.

regarding the "--message-format json" it does not work for listing.

it works only for running

parthopdas · 2023-02-08T20:09:00Z

i am really thankful to the folks for jumping in with their comments / perspectives in <24hrs. i love it ❤️.

however i also feel a lack of direction of what needs to be done. there is a 'pressing business need' so to speak and i do wish to do the right thing but in phase wise and balanced manner - prefer not to boil the ocean first up & sequentially.

@petrochenkov @matklad @Manishearth @ehuss @m-ou-se based on your comments above i recommend the following approach

divide this issue into 2 phases (you can trust me to see both through to completion)
1. unblock tooling requirement of listing tests (this change with perhaps modification of the format)
2. get consensus and push through a proper fix through "--message-format json" for test listing as well => through RFC proces.
start discussions on internal thread for long and short term phases => this will result in a --message-format RFC i believe
solicit feedback on Tracking issue for libtest JSON output #49359 for long and short term phases
incorporate feedback into this PR for formatting / nits / code changes (esp. so we dont break any existing consumers) => one of you approve this PR so tooling is unbocked.
implement the RFC and therefore the long term "proper" solution.

wdyt?

ehuss · 2023-02-08T20:14:05Z

Yea, what I think I and others are suggesting here is to add a JSON format for listing tests (instead of extending the textual format proposed here). Per the comments at #107785 (comment), I don't think we can change the existing output format, which I would agree with.

I would not expect there to be significant procedure for just introducing a JSON format for listing tests, the design space is not that large. Just take a little bit of time to see if anyone has any insight to contribute.

parthopdas · 2023-02-08T20:20:05Z

@ehuss may be i misunderstood.

are you suggesting the following poa?

keep console listing as is
extend "--message-format json" for use with --list (i.e. without runnng)
solicit feedback internals + Tracking issue for libtest JSON output #49359 to ensure we're not missing anything obvious?

if so that is simpler and i am happy to start on it.

@petrochenkov @matklad @Manishearth @m-ou-se let us know if you have any comments.

Manishearth · 2023-02-08T20:32:07Z

however i also feel a lack of direction of what needs to be done. there is a 'pressing business need' so to speak and i do wish to do the right thing but in phase wise and balanced manner - prefer not to boil the ocean first up & sequentially.

I think you have mismatched expectations as to the speed at which things move here.

Any feature addition, even if it's just a flag, is going to need to be properly thought through, because it's going to be supported forever. This is not something we take lightly. I think it's very unlikely that this PR will be merged without significant further discussion. We don't need to boil the ocean, but we do need to make sure we understand the design space and plan accordingly.

This applies doubly so given that the test runner is not quite owned by any team that can take responsibility here.

I don't see a "pressing business need" at all. I think this is an interesting feature to have, but by no means super urgent.

Also, I recommend checking out cargo nextest; an unofficial test runner, which may be more amenable to quick feature requests if you have a pressing need.

parthopdas · 2023-02-08T20:40:18Z

ok @Manishearth how do you recommend i proceed? like i said i want to do the right thing.

also i am happy for it to take as long as it does.

is this plan good enough for you?

keep console listing as is (revert the console.rs changes from this PR)
extend "--message-format json" for use with --list (i.e. without runnng)
solicit feedback internals + Tracking issue for libtest JSON output #49359 to ensure we're not missing anything obvious?
if all is well, push this PR through but with --message-format json and not console listing.

regarding the other points nextest doesn't work for me because it requires users to add another package - rust-analyzer.vs or any other tool for that matter, should work out of the box i.e. 0 friction.

for now we can keep aside whether this is a critical or a good to have - that depends on how you see things.

Manishearth · 2023-02-08T21:03:49Z

I think for now just drop any code changes, and work on soliciting feedback and coming up with a holistic plan for all of the things involved. Make an internals post and then an RFC based on the feedback you get there (and the feedback you've gotten here so far)

There's no need to work on the --message-format=json stuff just yet until we have a plan

parthopdas · 2023-02-08T21:12:12Z

@Manishearth okay sure.

so the new plan is:

can you point me to the 'internals forum"? is there an example / template post you'd like me to follow?
my initial post will start on the same lines i.e. business need, current inadequacy, recommendations to json format for listing and pointer this pr and Tracking issue for libtest JSON output #49359
give it a week, draft up a RFC (please point me to the process) based on the discussion
take it forward from there.

okay with you?

Manishearth · 2023-02-08T21:24:49Z

@parthopdas internals.rust-lang.org

You could post a "pre-RFC" using the RFC template but not necessarily fillign in every section

But actually I suggest just making a post and talk about the motivation, and solicit other motivations in the same area so you know what the set of needs are. And see what people propose as designs.

One concern I have is that you seem very very new to these processes, and it might be better if you can find someone more familiar who has time to help you shepherd this. I cannot.

okay with you?

FWIW i'm not the person you need to convince, you need to convince the community and the teams 😄

parthopdas · 2023-02-08T21:29:54Z

okay i'll start on it. thanks for the discussions everyone. love you guys! ❤️

@Manishearth let me know if you have anyone in mind who can shepherd this through.

parthopdas · 2023-02-08T22:31:21Z

@Manishearth please take a look https://internals.rust-lang.org/t/pre-rfc-enhancing-cargo-test-list-for-use-by-ide-test-explorers-runners/18308

Manishearth · 2023-02-08T22:37:40Z

regarding the other points nextest doesn't work for me because it requires users to add another package - rust-analyzer.vs or any other tool for that matter, should work out of the box i.e. 0 friction.

Actually, this doesn't apply: the entire purpose of this feature is to work with other third party packages (this wasn't clear to me earlier). Those packages have other methods available to them for doing test discovery: looking at the code in the file, or using runners like nextest. They can choose to include a nextest binary if they want.

Manishearth · 2023-02-08T22:38:32Z

Let me put my comment there

Manishearth · 2023-02-08T22:40:16Z

cc @calebcartwright : You seem to maintain vscode-rust-test-adapter, do you think you can help here?

Manishearth · 2023-02-08T22:42:15Z

Also cc @matklad who probably has a better idea of IDE integration stuff

calebcartwright · 2023-02-08T23:40:53Z

cc @calebcartwright : You seem to maintain vscode-rust-test-adapter, do you think you can help here?

I confess I'm a bit I'm unsure whether we're supposed to continue the dialog here or shift to internals, but will respond here for now.

From my pov I think it's fair to say that there's a lot that's already technically possible with the existing cargo test ... surface as evidenced by existing editor/ide tooling. However, it's absolutely true that it's rather painful and brittle for those integrations to get the requisite data, and I did have to rely on some kludges even for the relatively minimal set of capabilities provided by the vscode-rust-test-adapter extension.

I'd personally love to see some libtest enhancements that improve machine-friendliness to make certain test-related ide/editor features more feasible (which imo should not entail more textual output and parsing) because I think those types of features can play a non-insignificant factor in the overall developer experience with a language.

I'm not definitively sure who owns libtest either (sounds like there may have been plans to establish a test team?). I could probably make time to help assist an effort to make improvements, though I wouldn't have the bandwidth to fully drive the effort either.

(edit): I think it's also worth noting that the existing test listing info does include line numbers for doc tests, just not unit tests nor integration tests. I recognize that's likely due to some behind the scenes technicalities, but regardless of the editor/ide-centric impetus for this particular proposal, I definitely think it's worth at least a discussion on having the ability to do the same for unit and integration tests, if for no reason other than consistency.

parthopdas · 2023-02-09T03:11:24Z

@Manishearth what does it entail to take up 'ownership' of the testlib aspects?

i am happy to contribute to that and other areas depending on my needs but what in addition to that?

Manishearth · 2023-02-09T03:33:28Z

No, it's about team ownership.

Right now I think it mostly falls between libs and devtools. The creation of a formalized devtools subteam would be nice for it, but it would need to be run or at least advised by someone familiar with how the Rust project works. If you can find someone willing to take that on, we can try something.

In general libtest has been considered "done" (or, low priority) for a while which is why there hasn't ever been much motion on it. There's the custom test frameworks RFC (tracking issue) that didn't ever get driven further and that would be a nice task for a team picking this up, but kinda big.

parthopdas · 2023-02-09T04:58:10Z

since we are on the topic - i'd state my opinion as well on custom test frameworks RFC 50297 and a proposal for it jrenner proposal

i like the current approach better: doc / bench / tests with just 1 attribute [#test] with ability to extend e.g. test-case / quickcheck crates. even there while doc is an innovation but bench is also probably not required. RFC 50297 imho is overcomplicating testability aspects of rust code.

We could stabilize #[bench] and extend libtest with setup/teardown and other requested features.

i think this is the way to go - which is what we appear to be doing.

curiously the RFC is missing a prior art section. and while other ecosystems do have custom test frameworks (e.g. .net world has the xunit family) it is not to the extent the rfc is proposing.

imho end goal of testing is to rapidly provide feedback on the architecture itself. harder something is to test, lesser you can prove its correctness and more the complexity will seep into the end user experience.

i am a believer of 1000s of fine grained unit tests, 100ish of integration tests and 10s of end to end Acceptance tests. i cover that philosophy in bunch of talks at YT @parthopdas. simpler is better - lesser the constraints, the more degrees of freedom, the more scenarios can be covered (e.g. if just one test provides which in itself is complex, doesn't meet my needs what then? in addition more complex you make a test framework (e.g. setup teardown), the more complex testing itself becomes and lesser of it we do.

what i would love to see (and willing to make happen) is investment in tools that make testing experience sexy & a breeze - for the very same reasons.

case in point: ncrunch but for rust. and i believe with just minor tweaks to the existing infra, we have everything we need from the current testlib implementation.

rust-lang#107307 Adding additional information to "cargo test --list"…

dda61d8

… output earlier: p2dchecks::fibonacci_test::case_1: test now: // test test_type | ignored_or_not | location_info p2dchecks::fibonacci_test::case_1: test | false | src\lib.rs:57:8: 57:19

rustbot assigned petrochenkov Feb 8, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 8, 2023

parthopdas added 2 commits February 7, 2023 22:00

rust-lang#107307 Adding additional information to "cargo test --list"…

5c9c7a1

… output fixing build break

rust-lang#107307 Adding additional information to "cargo test --list"…

6a13a85

… output fixing build break

rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Feb 8, 2023

parthopdas added 4 commits February 7, 2023 23:52

rust-lang#107307 Adding additional information to "cargo test --list"…

13b0560

… output attempting to fix ordering issue

rust-lang#107307 Adding additional information to "cargo test --list"…

37a78c8

… output wtf am i doing?

rust-lang#107307 Adding additional information to "cargo test --list"…

801c86b

… output wtf am i doing (redux)?

rust-lang#107307 Adding additional information to "cargo test --list"…

a0839e4

… output fixing tests

petrochenkov reviewed Feb 8, 2023

View reviewed changes

parthopdas added 2 commits February 8, 2023 08:49

rust-lang#107307 Adding additional information to "cargo test --list"…

7588360

… output taking in PR comments.

rust-lang#107307 Adding additional information to "cargo test --list"…

6dffee4

… output fixing build break.

Manishearth removed the T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. label Feb 8, 2023

rust-lang#107307 Adding additional information to "cargo test --list"…

6a3ccf5

… output making the delimiter consistent.

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 8, 2023

rustbot added T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 8, 2023

m-ou-se added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 8, 2023

parthopdas closed this Feb 8, 2023

parthopdas mentioned this pull request Feb 19, 2023

Implementing "<test_binary> --list --format json" for use by IDE test explorers / runners rust-lang/compiler-team#592

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#107307 Adding additional information to "cargo test --list" output #107785

#107307 Adding additional information to "cargo test --list" output #107785

parthopdas commented Feb 8, 2023 •

edited

Loading

rustbot commented Feb 8, 2023

rustbot commented Feb 8, 2023

petrochenkov Feb 8, 2023

parthopdas Feb 8, 2023

petrochenkov commented Feb 8, 2023

matklad commented Feb 8, 2023

parthopdas commented Feb 8, 2023

Manishearth commented Feb 8, 2023 •

edited

Loading

Manishearth commented Feb 8, 2023

parthopdas commented Feb 8, 2023 •

edited

Loading

ehuss commented Feb 8, 2023

parthopdas commented Feb 8, 2023 •

edited

Loading

parthopdas commented Feb 8, 2023 •

edited

Loading

ehuss commented Feb 8, 2023

parthopdas commented Feb 8, 2023 •

edited

Loading

Manishearth commented Feb 8, 2023 •

edited

Loading

parthopdas commented Feb 8, 2023 •

edited

Loading

Manishearth commented Feb 8, 2023 •

edited

Loading

parthopdas commented Feb 8, 2023

Manishearth commented Feb 8, 2023

parthopdas commented Feb 8, 2023

parthopdas commented Feb 8, 2023

Manishearth commented Feb 8, 2023

Manishearth commented Feb 8, 2023

Manishearth commented Feb 8, 2023

Manishearth commented Feb 8, 2023

calebcartwright commented Feb 8, 2023 •

edited

Loading

parthopdas commented Feb 9, 2023

Manishearth commented Feb 9, 2023

parthopdas commented Feb 9, 2023

#107307 Adding additional information to "cargo test --list" output #107785

#107307 Adding additional information to "cargo test --list" output #107785

Conversation

parthopdas commented Feb 8, 2023 • edited Loading

earlier:

now:

rustbot commented Feb 8, 2023

rustbot commented Feb 8, 2023

petrochenkov Feb 8, 2023

Choose a reason for hiding this comment

parthopdas Feb 8, 2023

Choose a reason for hiding this comment

petrochenkov commented Feb 8, 2023

matklad commented Feb 8, 2023

parthopdas commented Feb 8, 2023

Manishearth commented Feb 8, 2023 • edited Loading

Manishearth commented Feb 8, 2023

parthopdas commented Feb 8, 2023 • edited Loading

ehuss commented Feb 8, 2023

parthopdas commented Feb 8, 2023 • edited Loading

parthopdas commented Feb 8, 2023 • edited Loading

ehuss commented Feb 8, 2023

parthopdas commented Feb 8, 2023 • edited Loading

Manishearth commented Feb 8, 2023 • edited Loading

parthopdas commented Feb 8, 2023 • edited Loading

Manishearth commented Feb 8, 2023 • edited Loading

parthopdas commented Feb 8, 2023

Manishearth commented Feb 8, 2023

parthopdas commented Feb 8, 2023

parthopdas commented Feb 8, 2023

Manishearth commented Feb 8, 2023

Manishearth commented Feb 8, 2023

Manishearth commented Feb 8, 2023

Manishearth commented Feb 8, 2023

calebcartwright commented Feb 8, 2023 • edited Loading

parthopdas commented Feb 9, 2023

Manishearth commented Feb 9, 2023

parthopdas commented Feb 9, 2023

parthopdas commented Feb 8, 2023 •

edited

Loading

Manishearth commented Feb 8, 2023 •

edited

Loading

parthopdas commented Feb 8, 2023 •

edited

Loading

parthopdas commented Feb 8, 2023 •

edited

Loading

parthopdas commented Feb 8, 2023 •

edited

Loading

parthopdas commented Feb 8, 2023 •

edited

Loading

Manishearth commented Feb 8, 2023 •

edited

Loading

parthopdas commented Feb 8, 2023 •

edited

Loading

Manishearth commented Feb 8, 2023 •

edited

Loading

calebcartwright commented Feb 8, 2023 •

edited

Loading