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

Quick fix to get new Scala collections compatibility in test. #228

Merged

Conversation

JanBessai
Copy link
Contributor

@JanBessai JanBessai commented Jul 18, 2018

As described in #225 .

@JanBessai
Copy link
Contributor Author

The deprecated breakout in InvokerConcurrency test can be replaced by toList. It is just for performance to convert an intermediate set to a list on-the-fly instead of in an additional step. In the test, performance is not an issue (especially not with 1000 entries), so it can be removed.

compiler.assertNMeasuredStatements(11)
// 2 statements for the two applies in Seq, one for each literal which is 6, one for the operation passed to yield.
// Depending on the collections api version, there can be additional implicit canBuildFrom statements.
compiler.assertAtLeastNStatements(9)
Copy link
Member

Choose a reason for hiding this comment

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

We can check the version of Scala here and expect different number of statements based on it:

   val expectedStatementsCount = if (ScoverageCompiler.ShortScalaVersion < "2.13") 11 else 9
   compiler.assertNMeasuredStatements(expectedStatementsCount)

@@ -33,7 +32,7 @@ class InvokerConcurrencyTest extends FunSuite with BeforeAndAfter {
Future {
Invoker.invoked(i, measurementDir.toString)
}
}(breakOut)
}.toList
Copy link
Member

Choose a reason for hiding this comment

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

Actually we don't need a list here, set would be good (val futures: Set[Future[Unit]] ...)

@gslowikowski
Copy link
Member

Thank you @JanBessai for your PR.
I've proposed two improvements (see my comments in the code), can you apply them? Additionally, ScoverageCompiler.assertAtLeastNStatements method is not needed and can be removed.

@JanBessai
Copy link
Contributor Author

Done. I've also removed some deprecated calls to prevent future breakage.

@gslowikowski gslowikowski merged commit 2872e73 into scoverage:scala-2-13-0-M4 Jul 22, 2018
@gslowikowski
Copy link
Member

Thank you Jan

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