compiletest: Report filtered and --skip-ed tests in metrics #155555
compiletest: Report filtered and --skip-ed tests in metrics #155555jyn514 wants to merge 4 commits into
Conversation
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in src/tools/compiletest cc @jieyouxu |
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
2235bed to
c4f30fb
Compare
This comment has been minimized.
This comment has been minimized.
c4f30fb to
a04daaf
Compare
This comment has been minimized.
This comment has been minimized.
a04daaf to
b35ca65
Compare
This comment has been minimized.
This comment has been minimized.
| #[derive(serde_derive::Deserialize, Debug)] | ||
| pub struct TestOutcome { | ||
| pub name: String, | ||
| pub exec_time: Option<f64>, |
There was a problem hiding this comment.
Do we standardize on a particular unit for time in this file? Maybe time_secs, or add comments indicating expected unit?
| } | ||
|
|
||
| #[derive(serde_derive::Deserialize, Debug)] | ||
| pub struct BenchOutcome { |
There was a problem hiding this comment.
Huh, I didn't know compiletest ever ran benchmarks... maybe I'm misreading though and some of these are really not from compiletest? Maybe we can add a comment on mod compiletest indicating the mixed origin?
There was a problem hiding this comment.
This is output that compiletest is parsing, not that compiletest is emitting. I'll leave a comment.
There was a problem hiding this comment.
ah no I'm wrong ... I'm not sure why this is here. I think left over from before the refactor away from libtest?
There was a problem hiding this comment.
in any case, I'd prefer to hear from a reviewer they're ok with the overall approach before I spend a bunch of time doing cleanups.
There was a problem hiding this comment.
AFAIK this is largely mimicking libtest JSON output to minimize the compiletest own test executor change (so that bootstrap libtest metric parsing can consume JSON output directly, and we could compare/swap the executors at the time) rather than compiletest actually emitting benchmark output (not that compiletest supports benchmarks in any case)
| match outcome { | ||
| TestOutcome::Passed => "pass".to_string(), | ||
| TestOutcome::Failed => "fail".to_string(), | ||
| TestOutcome::FilteredOut => "ignore".to_string(), |
There was a problem hiding this comment.
Should this be ignore (filtered out)?
c2f5e3c to
1552ae4
Compare
This comment has been minimized.
This comment has been minimized.
|
(Will try to get to this next Monday) |
There was a problem hiding this comment.
I feel like I need to think a bit more on how these pieces fit together. I don't necessarily feel like what you have here is problematic, but I feel like compiletest may need some messaging to make it easier to (self-)test 🤔
I'll need to do a second pass approx. this weekend.
| @@ -10,6 +10,7 @@ use std::io::{BufRead, BufReader, Read, Write}; | |||
| use std::process::ChildStdout; | |||
| use std::time::Duration; | |||
|
|
|||
| use build_helper::metrics::compiletest::*; | |||
There was a problem hiding this comment.
Nit: can you use exhaustive imports instead of glob imports here, just so it's more obvious where the names are coming from?
| @@ -35,7 +35,7 @@ test = false | |||
| cc = "=1.2.28" | |||
| cmake = "=0.1.54" | |||
|
|
|||
| build_helper = { path = "../build_helper" } | |||
| build_helper = { path = "../build_helper", features = ["metrics"] } | |||
There was a problem hiding this comment.
Remark: hm, I seem to recall this kinda defeats the purpose of having this feature. I thought that for bootstrap build time reasons, we didn't want to include the metrics functionality unless it's actually needed? Though I could be misremembering.
Might be worth double-checking the build time of bootstrap.
| BenchOk, | ||
| Failed, | ||
| Ignored { reason: Option<&'a str> }, | ||
| FilteredOut, |
There was a problem hiding this comment.
Question: can you say more on the distinction between ignored / filtered out? Is ignored intended to be like, //@ ignore-test or whatever, but filtered out more like "this is potentially eligible to run but we don't search there in the first place"?
|
@rustbot reroll |
View all comments
Ferrocene depends on these metrics for our certification. For various reasons our CI invokes bootstrap with
--skipflags, so we want to report ignored tests even if they're ignored manually with--skip.To make sure this doesn't regress, add an integration test suite for compiletest itself. This can't fit into the existing
compiletest-self-testsuite because it's testing the interface between bootstrap and compiletest, not just whether the tests pass or fail. Hopefully this will come in handy for more things in the future.Changes in this PR:
build_helper. DeriveDebugon all its types.FilteredOutmetric kind. Handle it appropriately (by not doing anything) in progress reporting, but save it to the metrics.json file.