sbt.JUnitXmlTestsListener removes part of test name if test name contains dot

[margussipria]

steps

sbt version: 1.3.0, 1.3.1, 1.3.2 (probably earlier)

problem

After spending hours figuring out why upgrade from 1.1.6 mangled our tests (scalatest/scalatest#1427 - really there should be migration info when JUnitXmlReportPlugin was added) I stared to test should run sbt's reporter or ScalaTest's.

• When running with 1.1.6 our CI reported that we have 1318 tests, it combined both reports with TEST- and without and removed duplicates.
• When disabled sbt's writer test count dropped to 1235 (which is correct).
• When running only sbt.JUnitXmlTestsListener test count dropped to 1233.

Problem is is that test names having dot in them are splitted and only last part is written to XML, and couple of test names ended in domain name and only net was written to file. (and again CI removes duplicates).

There is quite many test classes containing dot and caused previously extra 83 tests reported as names where different.

expectation

Test names in JUnitXmlTestsListener not to be truncated.

notes

https://github.com/sbt/sbt/blob/develop/testing/src/main/scala/sbt/JUnitXmlTestsListener.scala#L117

[eed3si9n]

@margussipria Thanks for the report! Would you like to contribute a fix for this?

[margussipria]

There is 5 Test framework's supporter currently and I do not know if they all behave same way. And there is Zero tests for that listener?

My fix would be to remove .split('.').last from 2 lines and it would work for ScalaTest and probably for Spec2, but would really need to be tested for JUnit. (https://github.com/sbt/junit-interface/blob/master/src/main/java/com/novocode/junit/EventDispatcher.java#L62 creating event names and https://github.com/sbt/junit-interface/blob/master/src/main/java/com/novocode/junit/AbstractEvent.java#L40 is creating selector).

I wouldn't like to make fix for me hand over garbage to someone else.

As https://github.com/sbt/junit-interface is separate project, first .split('.').last
should be added here https://github.com/sbt/junit-interface/blob/master/src/main/java/com/novocode/junit/AbstractEvent.java#L40 (and hope that I haven't missed something with other listeners?) and then bump up version and at same time removing split from here?

[HyukjinKwon]

FYI, this is a duplicate of #2949

[margussipria]

I made manual tests with 4 different backends (didn't test original Specs - hope that nobody is using Scala 2.8.x with sbt 1.3+).

I found that with every backend there is this same problem, .split('.').last are totally unnecessary, expect when using JUnit 4 in plain mode. Even with Junit 4.11+ there is dynamic part of name for Parameterized test and if there is dot in name template it would fail. (I did find example for template here https://stackoverflow.com/questions/650894/changing-names-of-parameterized-tests/10143872#10143872)

With JUnit 5 there is new @DisplayName annotation that could be used to change name what ever one would like to use. Testing with String.compare would result only compare to be appear in generated xml.

Did also find that scalacheck is reporting test suite names multiple times, opening one for suite and for every tests. (but this is probably fault of scalacheck rerporter.

./target/test-reports/TEST-example.StringSpecification-0.xml
./target/test-reports/TEST-example.StringSpecification-1.xml
./target/test-reports/TEST-example.StringSpecification-2.xml
./target/test-reports/TEST-example.StringSpecification.xml # empty

[eed3si9n]

sbt/testing/src/main/scala/sbt/JUnitXmlTestsListener.scala

Lines 115 to 122 in dca44d5

	yield <testcase classname={name} name={
	e.selector match {
	case selector: TestSelector => selector.testName.split('.').last
	case nested: NestedTestSelector =>
	nested.suiteId().split('.').last + "." + nested.testName()
	case other => s"(It is not a test it is a ${other.getClass.getCanonicalName})"
	}
	} time={(e.duration() / 1000.0).toString}>

What about the name (classname attribute) in the above?

If sbt/junit-interface is the only test framework that's emitting fully qualified name, could we compare the value of name against selector.testName and see if it starts with name? That way junit users can keep using their build without changing anything.

[eed3si9n]

@HyukjinKwon Thanks for letting us know. I'll make sure to close both issues once this gets fixed.

[margussipria]

If there is need to keep junit-interface as it is now, for JUnit 4 there can be match with name and then remove name + one char, but with JUnit 5 there is only test class, not full path.

I tested with following code:

val regex = s"""^($name|${name.split('.').last})""".r
 val matchEnd = regex.findFirstMatchIn(selector.testName).map(_.end + 1).getOrElse(0)
selector.testName.substring(matchEnd)

that would change result as following:
class: example.Junit4Test
testname: example.Junit4Test.testSomeString
old-result: testSomeString
new-result: testSomeString (same - package and test class was removed)

class: example.Junit5Test
testname: Junit5Test.Test "a".compare("a")
old-result: compare("a")
new-result: Test "a".compare("a") (correct - only test class was removed)

class: example.Spec2Spec
testname: The Hello object should::say hello.extension
old-result: extension
new-result: The Hello object should::say hello.extension (correct - nothing was removed)

Then there is nested tests (only Scalatest implements NestedTestSelector)
in this case class name is example.Combiner
case class would have values NestedTestSelector("example.SetSpec", "A Set when empty should have size 0")
Current result would be SetSpec.A Set when empty should have size 0
But Scalatest's own writer will write A Set when empty should have size 0 but changing name attribute in <testcase> tag to example.SetSpec

[eed3si9n]

Ok, then the regex solution sounds like a good solution for non-nested tests.

But Scalatest's own writer will write A Set when empty should have size 0 but changing name attribute in <testcase> tag to example.SetSpec

Changing name to nested.suiteId() sounds fine to me.