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 support for Scala 3 #429

Merged
merged 2 commits into from
Jun 3, 2022
Merged

Conversation

pikinier20
Copy link
Contributor

Re-created the pull request to fire CI.

Continuation of #428

@pikinier20 pikinier20 force-pushed the scala3-documentation branch from d0976bf to 4848a22 Compare May 19, 2022 08:56
@pikinier20
Copy link
Contributor Author

The CI failed because munit 0.7.25 for Scala 3 was compiled against java 11. Most likely it was by mistake because the newest munit is compiled against Java 8

Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Thanks for following up with this @pikinier20!

The CI failed because munit 0.7.25 for Scala 3 was compiled against java 11. Most likely it was by mistake because the newest munit is compiled against Java 8

Ahh are you referring to 0.7.29? If so, feel free to just replace all the references of 0.7.25 to 0.7.29 then, especially if it will fix the CI for the 8 run.

EDIT: Let me update the deps, then you should be able to just rebase.

// to be published localled in order to test
coverageEnabled.value && scalaVersion.value == "3.1.2-RC1-bin-SNAPSHOT"
) {
} else if (coverageEnabled.value) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think instead of just checking to see if it's enabled here we should also check that it's not an older version of dotc? If not, they might end up thinking this will work and it will set -coverage-out but not do anything.

Copy link
Member

Choose a reason for hiding this comment

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

Nice everything looks 🟢 ! I think if we can change this up to be more precise, we can get this merged in.

@pikinier20
Copy link
Contributor Author

Thanks for following up with this @pikinier20!

The CI failed because munit 0.7.25 for Scala 3 was compiled against java 11. Most likely it was by mistake because the newest munit is compiled against Java 8

Ahh are you referring to 0.7.29? If so, feel free to just replace all the references of 0.7.25 to 0.7.29 then, especially if it will fix the CI for the 8 run.

EDIT: Let me update the deps, then you should be able to just rebase.

I've already force-pushed branch with updated deps. Now there's some problem with resolving source paths. I'm not sure what exactly went wrong

@pikinier20
Copy link
Contributor Author

pikinier20 commented May 19, 2022

The issue present on CI is reproducible locally. It looks strange: the plugin works under java 11+ but doesn't work under java 8. Something "eats" part of the path: instead of:
sbt-scoverage/src/sbt-test/scoverage/scala3-good/src/main/scala/
we get sbt-scoverage/src/sbt-test/scoverage/src/main/scala/GoodCoverage.scala

It looks like the directory in which the project is present, is missing :/

EDIT with further investigations:
The scoverage.coverage file generated after running tests is different between java 1.8 and 11.
The heading with file name on java 1.8: ../src/main/scala/GoodCoverage.scala.
The heading with file name on java 11: src/main/scala/GoodCoverage.scala.

@pikinier20
Copy link
Contributor Author

I think it's the problem with compiler option. I removed the plugin, set the flag manually and ran tests. The behavior is the same: on java 1.8 it produces wrong source paths.

@pikinier20 pikinier20 force-pushed the scala3-documentation branch from 4848a22 to ac56ad2 Compare May 24, 2022 08:44
@pikinier20 pikinier20 requested a review from ckipp01 May 26, 2022 10:13
Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for sending this in @pikinier20!

@ckipp01 ckipp01 merged commit 7c804ed into scoverage:V2 Jun 3, 2022
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.

2 participants