-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Implementing "<test_binary> --list --format json" for use by IDE test explorers / runners #108148
Conversation
r? @cuviper (rustbot has picked a reviewer for you, use r? to override) |
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
@Manishearth fyi |
@cuviper For background: The plan here only affects a preexisting unstable CLI option ( (though the TestDescAndFn changes are going to be a pain for nightly compiletest, oh well) |
@Manishearth this PR already contains changes to a bunch of tests. but if your comment refers to something else i am happy to fix them if you let me know how to find them. |
// end_line: end line of the test fn identifier. | ||
field("end_line", cx.expr_usize(sp, location_info.3)), | ||
// end_col: end column of the test fn identifier. | ||
field("end_col", cx.expr_usize(sp, location_info.4)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the end location really useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is my guess right now, i think i will be needing this for the UI adornments e.g. highlighting the symbol when clicked on the test explorer and adding code lens related glyphs.
ps: pretty sure responded to this. not sure why my comments got reset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see
@@ -231,6 +231,8 @@ pub fn expand_test_or_bench( | |||
&item.ident, | |||
)); | |||
|
|||
let location_info = cx.sess.source_map().span_to_locatio_info(item.ident.span); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe do
if sess.should_collapse_debuginfo(span) {
// Walk up the macro expansion chain until we reach a non-expanded span.
// We also stop at the function body level because no line stepping can occur
// at the level above that.
// Use span of the outermost expansion site, while keeping the original lexical scope.
span = rustc_span::hygiene::walk_chain(span, self.mir.span.ctxt());
}
to get to the macro invocation site rather than macro definition site? The same is done for debuginfo:
rust/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs
Lines 210 to 216 in 9556b56
if self.cx.tcx().should_collapse_debuginfo(span) { | |
// Walk up the macro expansion chain until we reach a non-expanded span. | |
// We also stop at the function body level because no line stepping can occur | |
// at the level above that. | |
// Use span of the outermost expansion site, while keeping the original lexical scope. | |
span = rustc_span::hygiene::walk_chain(span, self.mir.span.ctxt()); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good suggestion, i think.
sess is not available in source_span, so i added it on test.rs line 234 (the one you highlighted above). however there "self.mir" is not available. any ideas? sess here is ExtCtxt while in debuginfo.rs self is FunctionCtx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Didn't notice the self.mir
. I'm not sure how to replace it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i based my changes on span_to_string
in source_map.rs. also searched unsuccessfully for a way to do what you recommended in there and in tests.rs.
for now i'll close it unless someone can help out.
note on commit messages: there's no need to put the issue number at the beginning of each message, and you probably don't want commits to have the same message: have each one describe what it does (right now when looking at a list of commits you don't really see anything useful because they're too long and overlapping) |
Just for visibility, this should get an MCP, I don't see it being controversial, but compiler folks should know about it via that mechanism. |
drats! closed the PR by mistake. @#!$@$!!! |
Yeah, I mean there's no need for the commit header to be the PR title for each one; just have your short message be the main commit message |
@oli-obk please point me to the process - happy to do that. |
@oli-obk please can you approve this PR? |
Please rebase the merge commits away and squash your fixup commits (or just make one big commit, that's also ok). Do we have documentation somewhere for the existing json output of the test suite? |
@bors r+ |
Implementing "<test_binary> --list --format json" for use by IDE test explorers / runners Fixes rust-lang#107307 PR 1 of 2 - wiring up just the new information + implement the command line changes i.e. --format json + tests upcoming: PR 2 of 2 - clean up "#[cfg(not(bootstrap))]" from PR 1 As per the discussions on - MCP: https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Implementing.20.22.3Ctest_binary.3E.20--list.20--form.E2.80.A6.20compiler-team.23592/near/328747548 - preRFC: https://internals.rust-lang.org/t/pre-rfc-implementing-test-binary-list-format-json-for-use-by-ide-test-explorers-runners/18308 - FYI on Discord: https://discord.com/channels/442252698964721669/459149169546887178/1075581549409484820
@bors rollup=iffy |
⌛ Testing commit 3720753 with merge 0d88c5e8fdfd901740e353a249f17c1f8cf9d9c2... |
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
@bors retry |
bors CI failure, tree closed now. updates here https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/segfaults/near/342843524 |
☀️ Test successful - checks-actions |
Finished benchmarking commit (9d0eac4): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
Update the toolchain to use nightly-2023-04-16. Changes were related to the following changes to the toolchain: - rust-lang/rust#108944 - rust-lang/rust#108148 - rust-lang/rust#108471 - rust-lang/rust#109358 - rust-lang/rust#109849 - rust-lang/rust#109819 - rust-lang/rust#109718 - rust-lang/rust#109092 - rust-lang/rust#108856 - rust-lang/rust#105613 - rust-lang/rust#103042 - rust-lang/rust#109716 - rust-lang/rust#108340 - rust-lang/rust#102906 - rust-lang/rust#98112 - rust-lang/rust#108080
Fixes #107307
PR 1 of 2 - wiring up just the new information + implement the command line changes i.e. --format json + tests
upcoming:
PR 2 of 2 - clean up "#[cfg(not(bootstrap))]" from PR 1
As per the discussions on