Skip to content

Add infrastructure to run the JUnit tests of upstream Scala.js. #6365

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

Merged
merged 11 commits into from
May 27, 2019

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Apr 23, 2019

And fix a number of bugs, just enough to be able to compile and successfully run compiler/IntTest.scala.

Based on #6304 and #6366.

@sjrd sjrd requested a review from smarter April 23, 2019 16:29
Copy link
Member

@dottybot dottybot left a comment

Choose a reason for hiding this comment

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

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:

  1. Separate subject from body with a blank line
  2. When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:
  3. Limit the subject line to 72 characters
  4. Capitalize the subject line
  5. Do not end the subject line with a period
  6. Use the imperative mood in the subject line ("Add" instead of "Added")
  7. Wrap the body at 80 characters
  8. Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

@sjrd sjrd force-pushed the sjs-junit-tests branch 4 times, most recently from 5ae11f6 to 8f16d9e Compare April 24, 2019 14:17
@sjrd sjrd force-pushed the sjs-junit-tests branch from 8f16d9e to 589e48e Compare May 3, 2019 15:57
@sjrd
Copy link
Member Author

sjrd commented May 3, 2019

Rebased. Ready for review.

@nicolasstucki
Copy link
Contributor

@sjrd, it looks like you are missing a dependency in the bootstrapped test

@sjrd sjrd force-pushed the sjs-junit-tests branch from 589e48e to fb3c37d Compare May 6, 2019 11:27
@sjrd
Copy link
Member Author

sjrd commented May 6, 2019

@nicolasstucki Indeed, thanks. It's fixed now.

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

Reviewed the build changes.

if (!trgDir.exists) {
s.log.info(s"Fetching Scala.js source version $ver")
IO.createDirectory(trgDir)
new CloneCommand()
Copy link
Member

Choose a reason for hiding this comment

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

Why clone the repo instead of fetching the source jar like we do for scalajs-ir ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this is a test-only artifact, which is not published at all (nor binary jar nor sources jar).


import org.scalajs.ir.ScalaJSVersions

object ConstantHolderGenerator {
Copy link
Member

Choose a reason for hiding this comment

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

We already use sbt-buildinfo for doing something similar, and I think one code generator thing is already too much :)

// Replace the JVM JUnit dependency by the Scala.js one
libraryDependencies ~= {
_.filter(!_.name.startsWith("junit-interface"))
lazy val sjsJUnitTests = project.in(file("tests/sjs-junit")).
Copy link
Member

Choose a reason for hiding this comment

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

Add a documentation comment explaining the purpose of this project.

@sjrd sjrd force-pushed the sjs-junit-tests branch from fb3c37d to 7c1a5d2 Compare May 21, 2019 09:38
sjrd added 10 commits May 21, 2019 14:09
In dotty, `mangledString` does not mangle operators that are not at
the end of an identifier, nor characters that are not
JavaIdentifierPart's. This is OK for the JVM, but not the Scala.js
IR. This commit forces full encoding of all characters.
Except those related to non-native JS classes.
The assertion fails for `OFFSET$x` fields at the moment. We'll
re-enable it when that is taken care of.
Instead emit a placeholder `throw null`. Actually producing correct
code for `Match` nodes is deferred for now.
@sjrd sjrd force-pushed the sjs-junit-tests branch 2 times, most recently from cd9fed2 to 8695f5f Compare May 21, 2019 12:12
And run one test for now: `compiler/IntTest.scala`.
@sjrd sjrd force-pushed the sjs-junit-tests branch from 8695f5f to 282caf6 Compare May 21, 2019 12:14
@sjrd
Copy link
Member Author

sjrd commented May 21, 2019

@smarter Updated. I did not migrate to sbt-buildinfo, because it generates a case object, and that calls into static methods of the Scala2x trait Product, and Scala.js never emitted those static methods. So basically inheriting from a Scala2x trait does not work at the moment. Fixing that does not seem to be trivial, and should be left for a different PR. When that's done, we'll be able to migrate to sbt-buildinfo with sjrd/dotty@sjs-junit-tests...sjrd:use-sbt-buildinfo-in-sjs-junit-tests.

@smarter
Copy link
Member

smarter commented May 21, 2019

Fixing that does not seem to be trivial, and should be left for a different PR.

The easiest fix I think would be to change our Scala2x support to not go through these static methods, this raises cold performance concerns (#5928) but it should have the correct semantics and be consistent with how we handle Dotty trait methods.

@smarter
Copy link
Member

smarter commented May 21, 2019

Build changes LGTM. I'll defer to @nicolasstucki for the code changes :).

Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

The code changes LGTM

@sjrd sjrd merged commit dbac6dc into scala:master May 27, 2019
@sjrd sjrd deleted the sjs-junit-tests branch September 9, 2020 13:05
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.

4 participants