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

[detly]

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

[sunshowers]

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.

[sunshowers]

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?

[detly]

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.

[sirynka]

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);
	}

[sunshowers]

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.