Emit Scala.js JUnit bootstrappers for JUnit test classes. #6304
Conversation
There was a problem hiding this comment.
Hello, and thank you for opening this PR! 🎉
All contributors have signed the CLA, thank you! ❤️
Commit Messages
We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).
Please stick to these guidelines for commit messages:
- Separate subject from body with a blank line
- When fixing an issue, start your commit message with
Fix #<ISSUE-NBR>:- Limit the subject line to 72 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line ("Add" instead of "Added")
- Wrap the body at 80 characters
- Use the body to explain what and why vs. how
adapted from https://chris.beams.io/posts/git-commit
Have an awesome day! ☀️
|
@smarter Currently I get the following crash somewhere in sbt when I try IIUC, this happens because the new Any idea how I can fix that? Note that the strategy of creating those classes late is not new. We use exactly the same strategy in scalac, and no similar issue has popped up. |
|
No idea. Can you describe what the phase your adding does in more details? Does it end up creating classfiles ? Does Scala code refers to the symbols in these classfiles somehow ? How does sbt incremental compilation handles deletion of these classfiles in Scala 2 ? |
|
The phase the following thing: for each class with at least one method having a JUnit-related annotation, e.g., class FooTest {
@Test
def thisIsATest(): Unit = ...
}it generates an accompanying module that looks like object FooTest$scalajs$junit$bootstrapper extends org.scalajs.junit.Bootstrapper {
def tests(): Array[org.scalajs.junit.TestMetadata] =
Array(new org.scalajs.junit.TestMetadata("thisIsATest", new org.junit.Test()))
def invokeTest(instance: Object, name: String): Unit =
if ("thisIsATest" == name) instance.asInstanceOf[FooTest].thisIsATest()
else throw new NoSuchMethodError(name)
}That module is then emitted by the back-end, and hence it emits an No Scala code ever refers to the generated code. It is only instantiated through Scala.js' run-time reflection mechanism. In Scala 2, I believe learns when to delete that .class file because the back-end records what compilation unit emits what .class file, and so when it recompiles a .scala file (or finds that a .scala file is deleted), it deletes all associated .class files. |
Dotty does the same, and off-topic for this PR but I think this won't be the case in sbt 1.3 / zinc 1.3 anymore due to sbt/zinc#582 moving the call to |
sjrd
force-pushed
the
scalajs-junit-tests
branch
from
2cc356e to
ea4cc66
Compare
|
I've looked into this a bit more and I really can't figure out why this doesn't crash in the exact same way in scalac. I guess we could add a call to |
sjrd
force-pushed
the
scalajs-junit-tests
branch
3 times, most recently
from
e376e2a to
e9d9677
Compare
sjrd
changed the title
sjrd
force-pushed
the
scalajs-junit-tests
branch
from
e9d9677 to
90172ea
Compare
This is correct and it's important to have a list of all generated class files for the incremental compiler to work correctly. Generated class files can be used to know which class files need to be moved from one classes directory to another one and to disable the invalidation of previous symbols based on the relative path of the class file. |
|
@jvican It seems that, in that case, it is now impossible for any compiler plugin that generates additional classes to be correct wrt incremental compilation. Should there be an API for compiler plugins to advertise what |
| val name: TermName = termName("name") | ||
| } | ||
|
|
||
| private class MyDefinitions()(implicit ctx: Context) { |
There was a problem hiding this comment.
Can this stuff go into JSDefinitions instead ?
There was a problem hiding this comment.
Right now it could, but this phase really should go into a separate compiler plugin at some point. For scalac it is in its own nsc plugin. The reason is that you only want to enable it on a) the test configuration and b) only if you actually use JUnit tests; otherwise, it's a lot of work done for nothing.
So basically I wrote it in the compiler right now, but we should extract it in a compiler plugin at some point, and therefore I wrote it in a way that does not invade the rest of the codebase.
There was a problem hiding this comment.
Well, the compiler already has to handle JUnit tests in an earlier phase: https://github.com/lampepfl/dotty/blob/1c3b45a3d292fb97a3316cb3e6b118b2966d4d95/compiler/src/dotty/tools/dotc/transform/MixinOps.scala#L66
So it could persist this information somewhere to make this phase a no-op if the current compilation unit doesn't have any JUnit test. (And I'm not too worried about JUnit tests in non-test configuration, that's an edge-case).
There was a problem hiding this comment.
needsJUnit4Fix is only called for methods in traits, as a last resort of whether they need a mixin forwarder or not. Relying on this mechanism to store the information would miss a lot of methods, including @Test methods in classes, which are obviously the common case.
There was a problem hiding this comment.
I'd still like to have these definitions in JSDefinitions since it's the simplest way to make sure they're only computed once, perhaps in their own inner object to keep them separate from the rest.
|
|
||
| import dotty.tools.dotc.transform.MegaPhase._ | ||
|
|
||
| /** Generates JUnit bootstrapper classes for Scala.js. */ |
There was a problem hiding this comment.
Moar documentation of what is being generated and why please 🙇♂️
|
Documentation added. |
nicolasstucki
requested review from
smarter
and removed request for
nicolasstucki
sjrd
force-pushed
the
scalajs-junit-tests
branch
3 times, most recently
from
cfe5ba7 to
54d6d30
Compare
sjrd
force-pushed
the
scalajs-junit-tests
branch
from
54d6d30 to
b8aab91
Compare
sjrd
force-pushed
the
scalajs-junit-tests
branch
from
b8aab91 to
775d5b8
Compare
| val name: TermName = termName("name") | ||
| } | ||
|
|
||
| private class MyDefinitions()(implicit ctx: Context) { |
There was a problem hiding this comment.
I'd still like to have these definitions in JSDefinitions since it's the simplest way to make sure they're only computed once, perhaps in their own inner object to keep them separate from the rest.
|
|
||
| DefDef(sym, { | ||
| val metadata = for (test <- tests) yield { | ||
| val name = Literal(Constant(test.name.toString)) |
There was a problem hiding this comment.
Is this correct with respect to name-mangling ? More tests please :).
There was a problem hiding this comment.
More tests will come from upstream Scala.js, notably coming from https://github.com/scala-js/scala-js/tree/v1.0.0-M7/test-suite/js/src/test/scala/org/scalajs/testsuite/junit, once #6365 is merged and I get to enable those tests. See for example JUnitNamesTest for the name-mangling aspect.
sjrd
force-pushed
the
scalajs-junit-tests
branch
from
775d5b8 to
6871ce6
Compare
|
|
||
| DefDef(sym, { | ||
| val metadata = for (test <- tests) yield { | ||
| val name = Literal(Constant(test.name.toString)) |
There was a problem hiding this comment.
More tests will come from upstream Scala.js, notably coming from https://github.com/scala-js/scala-js/tree/v1.0.0-M7/test-suite/js/src/test/scala/org/scalajs/testsuite/junit, once #6365 is merged and I get to enable those tests. See for example JUnitNamesTest for the name-mangling aspect.
compiler/src/dotty/tools/backend/jvm/DottyBackendInterface.scala
Outdated
Show resolved
Hide resolved
sjrd
force-pushed
the
scalajs-junit-tests
branch
from
3433e3f to
9f6d249
Compare
…ngClass. The logic in `originalOwner` was not only looking up the original owner, but also going further to the enclosing class of that owner, so the name was pretty misleading. This commit separates the two operations in two methods. The actual `originalOwner` half of it is extracted in `SymDenotations`, as it will be needed by the Scala.js back-end as well.
This allows Scala.js for dotty to support normal JUnit tests.
|
Updated. I believe I've addressed all the comments. |
|
@smarter Hum you said "Otherwise" but there isn't anything associated with that. Did you forget to push a "Comment" button at some point? Or is it fine to merge as is? |
This allows Scala.js for dotty to support normal JUnit tests.
See the equivalent scalac phase at https://github.com/scala-js/scala-js/blob/v1.0.0-M7/junit-plugin/src/main/scala/org/scalajs/junit/plugin/ScalaJSJUnitPlugin.scala