Skipped tests not shown in jUnit output when status-level="all"

The jUnit XML report does not seem to include skipped tests, even when the status-level is set to "all" in .config/nextest.toml.

Using the default project generated by cargo init --lib, I have this in src/lib.rs:

pub fn add(left: usize, right: usize) -> usize {
    left + right
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    #[ignore]
    fn it_works() {
        let result = add(2, 2);
        assert_eq!(result, 4);
    }
}

...and this in .config/nextest.toml:

[profile.default]
status-level = "all"
final-status-level = "all"
fail-fast = false

[profile.default.junit]
path = "test-results.xml"

The generated XML is in target/nextest/default/test-results.xml as expected, but contains:

<?xml version="1.0" encoding="UTF-8"?>
<testsuites name="nextest-run" tests="0" failures="0" errors="0" uuid="83992729-11dc-43e7-b5e6-58baf30be039" timestamp="2023-06-08T02:13:52.673+00:00" time="0.000">
</testsuites>

Skipped tests are reported on stdout though:

nextest-test ⚬ cargo nextest run
    Finished test [unoptimized + debuginfo] target(s) in 0.00s
    Starting 0 tests across 1 binaries (1 skipped)
        SKIP [         ] nextest-test tests::it_works
------------
     Summary [   0.000s] 0 tests run: 0 passed, 1 skipped
        SKIP [         ] nextest-test tests::it_works

I had expected skipped tests to also be listed in the jUnit output.

For comparison, here's the output from Rust 1.69 using the JSON output and cargo2junit:

<?xml version="1.0" encoding="utf-8"?>
<testsuites>
  <testsuite id="0" name="cargo test #0" package="testsuite/cargo test #0" tests="1" errors="0" failures="0" hostname="localhost" timestamp="2023-06-08T02:38:27.464333560+00:00" time="0">
    <testcase name="tests::it_works" time="0">
      <skipped />
    </testcase>
  </testsuite>
  <testsuite id="1" name="cargo test #1" package="testsuite/cargo test #1" tests="0" errors="0" failures="0" hostname="localhost" timestamp="2023-06-08T02:38:27.464333560+00:00" time="0" />
</testsuites>

The command to generate that was:

cargo +1.69 test -- -Z unstable-options --format json --report-time | cargo2junit > test-results.xml

OS: Ubuntu 22.10
Cargo: 1.70.0 (ec8a8a0ca 2023-04-25)
Rustc: 1.70.0 (90c541806 2023-05-31)
Nextest: 0.9.53 (installed via cargo install for dev, pre-built binary for CI)

Hi there! Yes, we should have a reasonable depiction of skipped tests in the JUnit output.

Would you like to work on this? I'd love to work out a design with you—if you have a general suggestion, we can take it from there. In particular, should we expose this as a config option or just include skipped tests unconditionally?

Oof, I'm sorry, I did not see a notification for this! I am happy to chip away at it, and yes, let's start with at least a vague design.

should we expose this as a config option or just include skipped tests unconditionally

First off - do you have any particular policy (or just expectations) around backwards compatibility or unexpected output changes? Because that would more or less decide it ie. it'd need to be a default-to-current-behaviour config option. Otherwise I'd go with making it include the skipped tests unconditionally, to make it consistent with the stdout reporting and the docs.

I think we do generally want machine-readable output to be stable, modulo bugs. This seems like a case where it would be pretty surprising to users to suddenly start seeing lots of extra tests in their dashboards, so I think putting it behind a config option makes sense.

Seems like this bit is responsible for skipped tests. If we'd uncomment it and update the naming the feature would work.
But I have not checked how to hide it behind the config flag yet.

nextest/nextest-runner/src/reporter/aggregator.rs

Lines 192 to 203 in 6304b41

    
           TestEvent::TestSkipped { .. } => { 
        
               // TODO: report skipped tests? causes issues if we want to aggregate runs across 
        
               // skipped and non-skipped tests. Probably needs to be made configurable. 
        
               // let testsuite = self.testsuite_for(test_instance); 
        
               // 
        
               // let mut testcase_status = TestcaseStatus::skipped(); 
        
               // testcase_status.set_message(format!("Skipped: {}", reason)); 
        
               // let testcase = Testcase::new(test_instance.name, testcase_status); 
        
               // 
        
               // testsuite.add_testcase(testcase); 
        
           }

Yes, that looks right. So it seems like it should be easy to do, we just need to figure out the configuration UI surface for this.

	TestEvent::TestSkipped { .. } => {
	// TODO: report skipped tests? causes issues if we want to aggregate runs across
	// skipped and non-skipped tests. Probably needs to be made configurable.

	// let testsuite = self.testsuite_for(test_instance);
	//
	// let mut testcase_status = TestcaseStatus::skipped();
	// testcase_status.set_message(format!("Skipped: {}", reason));
	// let testcase = Testcase::new(test_instance.name, testcase_status);
	//
	// testsuite.add_testcase(testcase);
	}