Scala.js: Add neg tests for bad uses of reflect.Selectable.
#9611
Conversation
We add some infrastructure to be able to perform neg tests that are specific to Scala.js. Such tests require the scalajs-library jar and the dotty-libraryJS jar on the classpath, and must be compiled with the `-scalajs` option. We use this infrastructure to test existing compile errors reported for unsupported uses of `reflect.Selectable` in Scala.js.
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! ☀️
project/Build.scala
Outdated
| @@ -705,7 +708,8 @@ object Build { | |||
| // running the compiler, we should always have the bootstrapped | |||
| // library on the compiler classpath since the non-bootstrapped one | |||
| // may not be binary-compatible. | |||
| "dotty-library" -> packageBin.in(`dotty-library-bootstrapped`, Compile).value | |||
| "dotty-library" -> packageBin.in(`dotty-library-bootstrapped`, Compile).value, | |||
| "dotty-library-js" -> packageBin.in(`dotty-library-bootstrappedJS`, Compile).value, | |||
There was a problem hiding this comment.
If I run dotty-compiler/run, the bootstrappedJS library is now compiled whereas it wasn't before, this would slow down working in dotty in most cases where we're not working on scala.js support. Similarly, I should be able to run dotty-compiler/testCompilation tests/pos/foo without having to compile anything scala.js-related. So I suggest moving this stuff to another project, either an existing scala.js project like sjsJUnitTests or a new one as you prefer.
There was a problem hiding this comment.
It really doesn't slow anything down, because a) it gets compiled in parallel with dotty-library-bootstrapped and b) it takes roughly the same time. I see a difference of about 1 out of 9 seconds on my computers (my Windows at home and my Linux at work agree) between
a) dotty-library-bootstrapped/clean followed by dotty-compiler/packageAll, and
b) dotty-library-bootstrapped/clean and dotty-library-bootstrappedJS/clean followed by dotty-compiler/packageAll.
The alternative is to replicate a lot of infrastructure in a different project. There is currently no other project equipped to do that. This seems like a huge increase in complexity to avoid 1 second.
There was a problem hiding this comment.
There is currently no other project equipped to do that.
dotty-compiler-bootstrapped runs the regular tests + bootstrapped-only and staging-only tests, it works by overriding the packageAll task. You could either do the same in a different project or add the scalajs stuff to dotty-compiler-bootstrapped too, that way it doesn't impact non-bootstrapped tasks which would be acceptable.
There was a problem hiding this comment.
Alright. Adding to the bootstrapped tests only was not an option for me, because that makes my dev cycle much much longer, since I would have to rebuild the bootstrapped compiler for every single change.
I added a separate project instead.
project/Build.scala
Outdated
| @@ -1140,6 +1140,27 @@ object Build { | |||
| } | |||
| ) | |||
|
|
|||
| lazy val sjsCompilerTests = project.in(file("tests/sjs-compiler-tests")). | |||
There was a problem hiding this comment.
I would put it at the top-level instead of in the tests/ directory since that directory should only contain code that we feed to the compiler to test it (hmm I've just now realized that we also have tests/sjs-junit which violates this, can't we merge these two things in one project anyway?). Also feel free to add a -boostrapped version of this project if needed.
project/Build.scala
Outdated
| dependsOn(`dotty-compiler` % "test->test"). | ||
| settings( | ||
| commonNonBootstrappedSettings, | ||
| outputStrategy := Some(StdoutOutput), |
There was a problem hiding this comment.
Can we move this into thisBuildSettings or commonSettings to avoid having to repeat it in various projects?
This way, it is not necessary to compile `dotty-library-bootstrappedJS` unless we want to run the Scala.js-specific tests. This reduces test coverage, since those tests won't be run with the bootstrapped compiler anymore.
sjrd
force-pushed
the
sjs-neg-tests-infrastructure
branch
from
d9e3f3f to
eb54a18
Compare
|
All done. |
We add some infrastructure to be able to perform neg tests that are specific to Scala.js. Such tests require the scalajs-library jar and the dotty-libraryJS jar on the classpath, and must be compiled with the
-scalajsoption.We use this infrastructure to test existing compile errors reported for unsupported uses of
reflect.Selectablein Scala.js.