Re-enable `PluginCoverageScalaJsTest`, fix `dangling UndefinedParam` bug #464

armanbilge · 2022-05-31T06:15:52Z

This PR re-enables the PluginCoverageScalaJsTest that is currently being ignored. ~~Fortunately, it still passes :)~~

Fixes #447.

This reverts commit 8da1f01.

armanbilge · 2022-05-31T07:34:12Z

Strange, I could have sworn the test was passing before! But failure is good too, it might mean #447 is a regression and not a new issue.

armanbilge · 2022-05-31T07:39:04Z

build.sbt

-    .dependsOn(runtimeJVM % Test)
+    // we need both runtimes compiled prior to running tests
+    .dependsOn(runtimeJVM % Test, runtimeJS % Test)


Aha, it was some trickiness actually. No regression.

Ah this make sense. 👍🏼 good catch.

armanbilge · 2022-05-31T07:46:31Z

Ok, this is ready for review :) I'll work on #447 in a follow-up PR.

armanbilge · 2022-05-31T08:40:24Z

Actually, I am skeptical if this is invoking the compiler the right way yet. Partly because I can't reproduce #447, and also because I realized I didn't make any configuration changes besides adding the sjs compiler to the classpath, which surely won't activate it magically 😅

armanbilge · 2022-05-31T09:04:51Z

Ok, I know I'm going in circles here, but now that I think I'm invoking the compiler correctly, indeed the existing test is broken with the same issue as #447.

/tmp/scoverage_snippet16088674966675160284.scala:3: error: Found a dangling UndefinedParam at Position(file:/tmp/scoverage_snippet16088674966675160284.scala,3,46). This is likely due to a bad interaction between a macro or a compiler plugin and the Scala.js compiler plugin. If you hit this, please let us know.
object JSONHelper {

armanbilge · 2022-05-31T09:07:22Z

Politely pinging @exoego who PRed the original fix in #281. Now that I have the test working, if you are able to help at all would be much appreciated! Thanks :)

armanbilge · 2022-05-31T09:36:56Z

So far it seems like the problem is detecting Scala.js itself.

scalac-scoverage-plugin/plugin/src/main/scala/scoverage/ScoveragePlugin.scala

Lines 101 to 107 in a557126

    
           private val isScalaJsEnabled: Boolean = { 
        
             try { 
        
               rootMirror.getClassIfDefined("scala.scalajs.js.Any") != NoSymbol 
        
             } catch { 
        
               case _: Throwable => false 
        
             } 
        
           }

If I change that to return true in the catch, then the test passes (with a minor adjustment to number of measured statements).

I'm hopeless to get it to print out the error 😕

armanbilge · 2022-05-31T10:03:29Z

Ok, got a stack trace that's thrown when calling rootMirror. Any ideas?

java.lang.NullPointerException
        at scala.reflect.internal.SymbolTable.phase_$eq(SymbolTable.scala:243)
        at scala.reflect.internal.SymbolTable.pushPhase(SymbolTable.scala:247)
        at scala.reflect.internal.SymbolTable.exitingPhase(SymbolTable.scala:290)
        at scala.reflect.internal.Symbols$TypeHistory.phaseString(Symbols.scala:3850)
        at scala.reflect.internal.Symbols$TypeHistory.$anonfun$toString$1(Symbols.scala:3852)
        at scala.collection.Iterator$$anon$9.next(Iterator.scala:577)
        at scala.collection.IterableOnceOps.addString(IterableOnce.scala:1184)
        at scala.collection.IterableOnceOps.addString$(IterableOnce.scala:1179)
        at scala.collection.AbstractIterator.addString(Iterator.scala:1293)
        at scala.collection.IterableOnceOps.mkString(IterableOnce.scala:1129)
        at scala.collection.IterableOnceOps.mkString$(IterableOnce.scala:1127)
        at scala.collection.AbstractIterator.mkString(Iterator.scala:1293)
        at scala.reflect.internal.Symbols$TypeHistory.toString(Symbols.scala:3852)
        at java.base/java.lang.String.valueOf(String.java:2951)
        at scala.reflect.internal.SymbolTable.throwAssertionError(SymbolTable.scala:171)
        at scala.reflect.internal.Symbols$TypeHistory.<init>(Symbols.scala:3831)
        at scala.reflect.internal.Symbols$TypeHistory$.apply(Symbols.scala:3829)
        at scala.reflect.internal.Symbols$Symbol.info_$eq(Symbols.scala:1586)
        at scala.reflect.internal.Symbols$TypeSymbol.info_$eq(Symbols.scala:3256)
        at scala.reflect.internal.Symbols$Symbol.setInfo(Symbols.scala:1592)
        at scala.reflect.internal.Mirrors$Roots$RootClass.<init>(Mirrors.scala:313)
        at scala.reflect.internal.Mirrors$Roots.RootClass$lzycompute(Mirrors.scala:327)
        at scala.reflect.internal.Mirrors$Roots.RootClass(Mirrors.scala:327)
        at scala.reflect.internal.Mirrors$Roots$EmptyPackageClass.<init>(Mirrors.scala:336)
        at scala.reflect.internal.Mirrors$Roots.EmptyPackageClass$lzycompute(Mirrors.scala:342)
        at scala.reflect.internal.Mirrors$Roots.EmptyPackageClass(Mirrors.scala:342)
        at scala.reflect.internal.Mirrors$Roots.EmptyPackageClass(Mirrors.scala:282)
        at scala.reflect.internal.Mirrors$RootsBase.init(Mirrors.scala:256)
        at scala.tools.nsc.Global.rootMirror$lzycompute(Global.scala:75)
        at scala.tools.nsc.Global.rootMirror(Global.scala:73)
        at scoverage.ScoverageInstrumentationComponent.liftedTree1$1(ScoveragePlugin.scala:104)

armanbilge · 2022-05-31T10:07:52Z

Aha, I think making it lazy does the trick.

armanbilge · 2022-06-01T18:09:35Z

I can confirm the fix (as released in 2.0.0-M7+1-9cfca69b-SNAPSHOT) works in downstreams 👍

ckipp01

This looks good @armanbilge. Just a quick question on the buildInfo module though.

ckipp01 · 2022-06-03T13:33:02Z

build.sbt

@@ -179,6 +181,16 @@ lazy val reporter =
    )
    .dependsOn(domain, serializer)

+lazy val buildInfo =


How come we have a separate build target for buildInfo instead of just including the buildInfo stuff inside of the plugin module and enabling the BuildInfoPlugin there?

Aha, yes I suppose we could. I did this because the BuildInfo was only needed in the Test scope and there's currently no way to configure that (see sbt/sbt-buildinfo#186). But I guess here if we "leak" the BuildInfo as part of the shipped plugin, it's probably not a big deal?

was only needed in the Test scope

Yea, that's sort of what I expected. No worries. Let's leave it as is.

ckipp01 · 2022-06-03T13:34:01Z

build.sbt

-    .dependsOn(runtimeJVM % Test)
+    // we need both runtimes compiled prior to running tests
+    .dependsOn(runtimeJVM % Test, runtimeJS % Test)


Ah this make sense. 👍🏼 good catch.

ckipp01

Alrighty, LGTM. Thanks @armanbilge!

armanbilge added 8 commits May 29, 2022 21:10

Try re-enabling PluginCoverageScalaJsTest

585ed58

ScoverageCompiler refactoring to support SJS

61be139

Scala.js test is working again

e5bbd14

Remove unneeded dep

8da1f01

Try adding buildInfo to root

66de565

Set crossScalaVersions for buildinfo project

c13a068

Revert "Remove unneeded dep"

0c69003

This reverts commit 8da1f01.

Add another sjs test dep

7b9212b

plugin also depends on runtimeJS for tests

d8d2a1c

armanbilge commented May 31, 2022

View reviewed changes

armanbilge marked this pull request as draft May 31, 2022 08:40

Actually enable the sjs compiler plugin :)

aa833a4

armanbilge added 2 commits May 31, 2022 09:40

Fix expected measured statements 4 -> 2

09f3673

Only catch non-fatals

695ce2d

Make isScalaJsEnabled a lazy val

775828f

armanbilge changed the title ~~Re-enable PluginCoverageScalaJsTest~~ Re-enable PluginCoverageScalaJsTest, fix dangling UndefinedParam bug May 31, 2022

armanbilge marked this pull request as ready for review May 31, 2022 10:10

I think these deps are unneeded now

96136e5

armanbilge mentioned this pull request Jun 1, 2022

Make isScalaJsEnabled a lazy val #466

Merged

ckipp01 reviewed Jun 3, 2022

View reviewed changes

ckipp01 approved these changes Jun 3, 2022

View reviewed changes

ckipp01 merged commit 7eba8e1 into scoverage:main Jun 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable `PluginCoverageScalaJsTest`, fix `dangling UndefinedParam` bug #464

Re-enable `PluginCoverageScalaJsTest`, fix `dangling UndefinedParam` bug #464

armanbilge commented May 31, 2022 •

edited

Loading

armanbilge commented May 31, 2022

armanbilge May 31, 2022

ckipp01 Jun 3, 2022

armanbilge commented May 31, 2022

armanbilge commented May 31, 2022

armanbilge commented May 31, 2022

armanbilge commented May 31, 2022

armanbilge commented May 31, 2022

armanbilge commented May 31, 2022

armanbilge commented May 31, 2022

armanbilge commented Jun 1, 2022

ckipp01 left a comment

ckipp01 Jun 3, 2022

armanbilge Jun 3, 2022 •

edited

Loading

ckipp01 Jun 3, 2022

ckipp01 Jun 3, 2022

ckipp01 left a comment

Re-enable PluginCoverageScalaJsTest, fix dangling UndefinedParam bug #464

Re-enable PluginCoverageScalaJsTest, fix dangling UndefinedParam bug #464

Conversation

armanbilge commented May 31, 2022 • edited Loading

armanbilge commented May 31, 2022

armanbilge May 31, 2022

Choose a reason for hiding this comment

ckipp01 Jun 3, 2022

Choose a reason for hiding this comment

armanbilge commented May 31, 2022

armanbilge commented May 31, 2022

armanbilge commented May 31, 2022

armanbilge commented May 31, 2022

armanbilge commented May 31, 2022

armanbilge commented May 31, 2022

armanbilge commented May 31, 2022

armanbilge commented Jun 1, 2022

ckipp01 left a comment

Choose a reason for hiding this comment

ckipp01 Jun 3, 2022

Choose a reason for hiding this comment

armanbilge Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

ckipp01 Jun 3, 2022

Choose a reason for hiding this comment

ckipp01 Jun 3, 2022

Choose a reason for hiding this comment

ckipp01 left a comment

Choose a reason for hiding this comment

Re-enable `PluginCoverageScalaJsTest`, fix `dangling UndefinedParam` bug #464

Re-enable `PluginCoverageScalaJsTest`, fix `dangling UndefinedParam` bug #464

armanbilge commented May 31, 2022 •

edited

Loading

armanbilge Jun 3, 2022 •

edited

Loading