Skip to content
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

Add test reporter for Kotlin/JS #3757

Merged
merged 3 commits into from
Oct 23, 2024

Conversation

0xnm
Copy link
Contributor

@0xnm 0xnm commented Oct 16, 2024

This PR adds test reporter for Kotlin/JS. It is based on XUnit reporter provided by Mocha - XML will be generated anyway, irrespective of the TestModule.testReportXml option, because structured output is needed in order to be able to parse it.

So as a result it will be XML file + test result message, which can be seen in the tests.

Adding HTML reporter is not yet possible without using 3rd party packages, because the one provided OOTB by Mocha is not intended for CLI usage.

I also had to modify the Jvm.runSubprocess method, so it doesn't throw if exit code is not 0, but returns result. To comply with the old behavior, all the old call sites will just call getOrThrow on the result.

This change is needed, because if there is a test failure, node process will exit with code 1, but we still need to process and catching generic Exception is not an option (because maybe it is not because of the exit code).

Upd: Binary compatibility check now complains that Jvm.runSubprocess has a different return type, but I'm not sure if it should be a problem, because before it was simply returning Unit. But if it is a problem, I can introduce a new method instead.

@0xnm 0xnm force-pushed the add-test-reporter-for-kotlin-js branch 2 times, most recently from 056e76e to a2e69e5 Compare October 16, 2024 21:35
@lihaoyi
Copy link
Member

lihaoyi commented Oct 17, 2024

Upd: Binary compatibility check now complains that Jvm.runSubprocess has a different return type, but I'm not sure if it should be a problem, because before it was simply returning Unit. But if it is a problem, I can introduce a new method instead.

Yes let's give the new method a new name to preserve binary compatibility. The old one returning : Unit can just call the new one and ignore the return value

@0xnm 0xnm force-pushed the add-test-reporter-for-kotlin-js branch 2 times, most recently from acbacdd to 0eb561c Compare October 17, 2024 17:43
@lihaoyi
Copy link
Member

lihaoyi commented Oct 20, 2024

Seems like some of the mill.kotlinlib.js.KotlinJSNodeRunTests tests are failing

@0xnm 0xnm force-pushed the add-test-reporter-for-kotlin-js branch from 0eb561c to 6d4a4fb Compare October 20, 2024 15:19
@0xnm
Copy link
Contributor Author

0xnm commented Oct 20, 2024

Oh, yeah, indeed, thank for spotting it. I was thinking it was a flaky test from scalalib, but turns out not only it.

This is the case where type safety cannot be checked during the compile time, because Result.Failing is itself Exception.

Fixed now.

}

val moduleKind = this.moduleKind()
protected[js] def runJsBinary(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the name runKotlinJsBinary, since chances are high the user project may also have some Scala.JS or other JS modules.

@@ -457,7 +475,9 @@ trait KotlinJsModule extends KotlinModule { outer =>

override def testFramework = ""

override def kotlinJSBinaryKind: T[Option[BinaryKind]] = Some(BinaryKind.Executable)
override def kotlinJSRunTarget: T[Option[RunTarget]] = outer.kotlinJSRunTarget()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
override def kotlinJSRunTarget: T[Option[RunTarget]] = outer.kotlinJSRunTarget()
override def kotlinJsRunTarget: T[Option[RunTarget]] = outer.kotlinJsRunTarget()

@@ -20,6 +22,7 @@ object KotlinJsKotlinTestPackageModuleTests extends TestSuite {

object foo extends KotlinJsModule {
def kotlinVersion = KotlinJsKotlinTestPackageModuleTests.kotlinVersion
override def kotlinJSRunTarget = Some(RunTarget.Node)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
override def kotlinJSRunTarget = Some(RunTarget.Node)
override def kotlinJsRunTarget = Some(RunTarget.Node)

@lihaoyi lihaoyi merged commit 9bae771 into com-lihaoyi:main Oct 23, 2024
24 checks passed
@lefou lefou added this to the 0.12.0 milestone Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants