Run scripted tests on sbt 1.4.x + refactoring #10655

mkurz · 2021-01-22T22:26:07Z

As suggested once by Dale I now folded scripts/test-scripted-13x into scripts/test-scripted. I also can not see why we should have different scripts for different sbt versions... Also because this pull request now adds another sbt version,1.4.9, meaning the scripted tests will now be run against ~~1.2.x,~~ 1.3.x and 1.4.x.

I also defined fixed sbt versions in .travis.yml. Before that the job(s) that "Run scripted tests (a/b/c) for sbt 1.x" would just use the sbt version set by interplay (See Line -> Line -> Line). IMHO setting the versions hardcoded in .travis.yml is more readable (took me a while to figure out the default comes from interplay), safer (what if interplay upgrades to 1.3.x or newer, but nobody updates the .travis.yml description, when looking at the tests someone may still thinks that the tests are running for 1.2.x) and also easier to maintain (we can quickly test new sbt versions by just updating .travis.yml instead of releasing and upgrading interplay).

mkurz · 2021-01-27T16:18:15Z

~~Following scripted tests fail when running with sbt 1.4, however this was expected by me, because sbt 1.4 support is not done/merged yet (also #10711 not yet):~~ (All fixed 🎉)

[error] 	play-sbt-plugin/dev-mode
[error] 	play-sbt-plugin/evolutions-auto-apply-false
[error] 	play-sbt-plugin/evolutions-auto-apply-true
[error] 	play-sbt-plugin/evolutions-multiple-databases
[error] 	play-sbt-plugin/generated-keystore
[error] 	play-sbt-plugin/http-backend-system-property
[error] 	play-sbt-plugin/maven-layout-twirl-reload
[error] 	play-sbt-plugin/multiproject
[error] 	play-sbt-plugin/play-position-mapper
[error] 	play-sbt-plugin/routes-compiler-source-mapping
[error] 	play-sbt-plugin/shutdown-downing
[error] 	play-sbt-plugin/shutdown-happy-path

.travis.yml

mkurz · 2021-02-25T20:40:41Z

Backport #10723

mkurz · 2021-03-02T15:25:49Z

Now that #10711 got merged, I restarted the tests for this PR, let's see if all the tests pass when running against sbt 1.4

mkurz · 2021-03-03T08:28:38Z

~~Following scripted tests fail when running against sbt 1.4.x, but pass with 1.3.x~~ EDIT: Fixed!

[error] (scripted) Failed tests:
[error] 	play-sbt-plugin/multiproject
[error] 	play-sbt-plugin/play-position-mapper
[error] 	play-sbt-plugin/routes-compiler-source-mapping

Challenge accepted

mkurz · 2021-03-05T11:56:05Z

To make play-sbt-plugin/play-position-mapper work we need to wait for sbt 1.4.8, which ~~should be~~ got released this weekend. I had to patch sbt:
sbt/sbt#6352
1.4.x backport: sbt/sbt#6356
EDIT:
One more patch needed: sbt/sbt#6358
1.4.x backport: sbt/sbt#6359
EDIT 2: All fixed an released in sbt 1.4.8/1.4.9

…for sbt 1.4+

mkurz · 2021-03-11T15:24:21Z

dev-mode/sbt-plugin/src/main/scala/play/sbt/run/PlayReload.scala

+              // anymore before adding it to "allProblems", the field that eventually gets used by the Incomplete. (The transformation still takes place to show
+              // the mapped source file in the logs) Play however needs to know the mapped source file to display it in it's error pages for a nice dev experience.
+              // So the solution is that Play itself will try to transform the source file to the mapped file by running it "through" sourcePositionMappers:
+              Project


I think the comment says it all... So since sbt/zinc#931 (which made it into sbt 1.4.1) the left Incomplete of following compileResult

playframework/dev-mode/sbt-plugin/src/main/scala/play/sbt/run/PlayReload.scala

Lines 63 to 67 in 47f277b

val compileResult: Either[Incomplete, CompileSuccess] = for {

analysis <- reloadCompile().toEither.right

classpath <- classpath().toEither.right

} yield CompileSuccess(sourceMap(analysis), classpath.files)

compileResult.left.map(inc => CompileFailure(taskFailureHandler(inc, streams()))).merge

comes with Positions which are not transformed by the sourcePositionMappers config anymore. Here is the relevant code after that referenced pull request got applied for better context:
https://github.com/sbt/zinc/blob/v1.4.1/internal/zinc-compile-core/src/main/scala/sbt/internal/inc/LoggedReporter.scala#L131-L139
So since 1.4.1 sbt only transforms the position for log outputs... which is a problem for Play, so we have to make the Positions run through the sourcepositionmappers ourselves to show users the correct original soure of a problem in the error pages...

I also added two new tests for this new code here, which is relevant for sbt 1.4.1, see below.

Also I had to remove one scripted test which will always fail in sbt 1.4.1+, see routes-compiler-source-mapping/build.sbt below (I think you have to click "Load diff" in the GitHub UI to see the comment I left there). However the two new tests I added compensate the deleted one.

mkurz · 2021-03-11T15:31:18Z

...sbt-plugin/src/sbt-test/play-sbt-plugin/dev-mode-compile-and-config-error-source/build.sbt.1

-        val (status, body) = ScriptedTools.callUrl(path)
-        println(s"Resource at $path returned HTTP $status")
-        IO.write(destination, body)
-      }


No need to wrap this in a Future for these tests here... This was a mistake from beginning when I just copy/pasted the makeRequestAndRecordResponseBody task from the shutdown-happy-path scripted test where that Future is actually needed because it is a different test case.
As a result, in the test, we can also remove the 2 and 5 seconds sleeps after calling the command because now its blocking.

dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/dev-mode-compile-and-config-error-source/test

mkurz · 2021-03-11T15:40:20Z

dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/dev-mode-compile-and-config-error-source/test

 > makeRequestAndRecordResponseBody / appconf-fail.txt
 # First request takes a bit longer, therefore 5 seconds
-$ sleep 5000


Like mentioned makeRequestAndRecordResponseBody is blocking now, no need anymore for that sleep.

dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/dev-mode-compile-and-config-error-source/test

mkurz · 2021-03-11T15:50:49Z

dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/multiproject/build.sbt

@@ -64,7 +64,28 @@ TaskKey[Unit]("checkPlayCompileEverything") := {
    base / "playmodule" / "app" / "PlayModule.scala",
    base / "transitive" / "app" / "Transitive.scala"
  )
-  val allSources = analyses.flatMap(_.relations.allSources).map(_.toPath).sorted.map(_.toFile)
+  val allSources = analyses.flatMap(_.relations.allSources).map(_ match {


allSources returns VirtualFiles in sbt 1.4 (which has no toPath method) but a Java File in sbt <= 1.3... Therefore I had to apply the same workaround in this test we used in PR 10648: https://github.com/playframework/playframework/pull/10648/files#diff-e1d77acde8f8aee39e58b112462d82fdcbc8c2920092be5af01ed5b3a92ca64b

EDIT:
Actually I had the error log still open in an editor so this is what failed when running with sbt 1.4:

[error] /tmp/sbt_3797ffa5/build.sbt:67: error: value toPath is not a member of xsbti.VirtualFileRef [error] val allSources = analyses.flatMap(_.relations.allSources).map(_.toPath).sorted.map(_.toFile) [error] ^

mkurz · 2021-03-11T15:56:31Z

...st/play-sbt-plugin/routes-compiler-injected-routes-compilation-with-request-passed/build.sbt

@@ -5,6 +5,7 @@ lazy val root = (project in file("."))
  .enablePlugins(PlayJava)

 libraryDependencies ++= Seq(guice, specs2 % Test)
+libraryDependencies += "org.scala-lang.modules" %% "scala-collection-compat" % "2.3.2" // can be removed when dropping Scala 2.12


This an all changes below make the scripted tests behave on Scala 2.13 (until now they were just run with Scala 2.12...)

mkurz · 2021-03-11T15:59:29Z

dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/routes-compiler-source-mapping/build.sbt

-  val source = base / args(0)
-  val line   = Integer.parseInt(args(1))
-  val errors = Incomplete
-    .allExceptions(assertLeft(assertSome(Project.runTask(compile in Compile, state.value))._2.toEither))


I removed that test because it will never succeed anymore in sbt 1.4.1+ because Positions returned by the incomplete will not be transformed anymore by the sourcepositionmappers, see comment above.

dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/show-versions/test

mkurz · 2021-03-11T16:08:34Z

project/BuildSettings.scala

@@ -376,7 +376,6 @@ object BuildSettings {
      "-XX:MaxMetaspaceSize=300m",
      "-XX:HeapDumpPath=/tmp/",
      "-XX:+HeapDumpOnOutOfMemoryError",
-      s"-Dscala.version=$scala212",


This gets set by script/test-scripted now.

mkurz · 2021-03-11T16:09:17Z

scripts/publish-local

@@ -12,7 +12,7 @@ rm -rf $HOME/.ivy2/local
 end clean "CLEANED IVY LOCAL REPO"

 start publish-local "CROSS-PUBLISHING PLAY LOCALLY FOR SBT SCRIPTED TESTS"
-runSbt +publishLocal
+runSbt ";crossScalaVersions;crossSbtVersions;+publishLocal"


Just adding some output for debugging...

mkurz · 2021-03-11T17:13:50Z

@ignasi35 @octonato This pull request is finally ready to review!

As an example how a cron build will look like (except without sbt 1.2) you can have a look at
https://travis-ci.com/github/playframework/playframework/builds/219526837
which is the build of this commit:

(the commit before I added the cron conditions)

ignasi35

This is great work! Thanks @mkurz

I only have a concern re the cost of running so many scripted tests in the CRON jobs. I'll merge as is but we should keep an eye on the weekend cron jobs to get a better idea of how much cost/benefit this brings.

ignasi35 · 2021-03-12T10:14:36Z

.travis.yml

+  - test-sbt-1.3.x
+  - cron-test-sbt-1.3.x
+  - cron-test-sbt-1.4.x


Suggested change

- test-sbt-1.3.x

- cron-test-sbt-1.3.x

- cron-test-sbt-1.4.x

- test-sbt-1.3.x-2.12

- cron-test-sbt-1.3.x-2.13

- cron-test-sbt-1.4.x

Indicating the difference (scala version) to make it clearer what's the version tested on every PR.

Not a blocker. Let's solve this elsewhere.

ignasi35 · 2021-03-12T10:17:25Z

.travis.yml

+    # Test against sbt 1.4.x, but only for cron builds
+    - stage: cron-test-sbt-1.4.x
+      name: "Run tests for 1.4.x and Scala 2.12.x"
+      script: scripts/test-scripted $SCRIPTED_SBT_1_4 $TEST_SCALA_2_12
+      if: type = cron
+      workspaces:
+        use: published-local
+    - name: "Run tests for 1.4.x and Scala 2.13.x"
+      script: scripts/test-scripted $SCRIPTED_SBT_1_4 $TEST_SCALA_2_13


This is rather expensive and I'm not 100% sure we should extend the support matrix in all possible axis (JDK8/11, sbt1.3/1.4, scala2.12/2.13).

I think running only 1.4.x for 2.13 could be enough. I think users of sbt 1.4 may be more likely to be early adopters in general and, therefore, users of 2.13 already.

Again, not sure whether we should keep both jobs or not...

dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/dev-mode-compile-and-config-error-source/test

ignasi35 · 2021-03-12T10:21:00Z

dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/dev-mode-compile-and-config-error-source/test

@@ -30,96 +30,152 @@ $ delete build.sbt.1
 ################
 $ copy-file conf/application.conf application.conf.good
 $ copy-file application.conf.bad conf/application.conf
+# Give CI/docker/Java thread enough time to detect file change
+$ sleep 1200
 > makeRequestAndRecordResponseBody / appconf-fail.txt
 # First request takes a bit longer, therefore 5 seconds


This comment is obsolete (the sleep is gone)

I was referring to # First request takes a bit longer, therefore 5 seconds

dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/dev-mode-compile-and-config-error-source/test

ignasi35 · 2021-03-12T10:25:02Z

dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/dev-mode-compile-and-config-error-source/test

+#############################################################################################################################################
+# Invalid twirl template (parsing index.scala.html file is OK, but compiling target/scala-*/twirl/main/views/html/index.template.scala fails)
+# Actually this tests if the sourcePositionMappers set up by sbt-twirl is called and works correctly
+#############################################################################################################################################


These comments are great. Help the maintenance of the tests as they explain the expectation and use case very clearly. 👏🏼

dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/show-versions/test

mkurz · 2021-03-13T11:51:58Z

@ignasi35 thanks a lot for the review! I removed the obsolete comment.

I only have a concern re the cost of running so many scripted tests in the CRON jobs

Like you said, lets merge and observe, we can always change/remove jobs from the matrix.

mkurz force-pushed the sbt14_refactoring branch from fac3d59 to f1961cf Compare January 22, 2021 22:37

mkurz added the status:block-merge label Jan 22, 2021

mkurz force-pushed the sbt14_refactoring branch 3 times, most recently from 98b75b1 to 2013eb0 Compare January 25, 2021 23:12

mkurz added this to the 2.8.8 milestone Feb 15, 2021

mkurz force-pushed the sbt14_refactoring branch 2 times, most recently from 4801f43 to 7acb9b2 Compare February 25, 2021 11:12

mkurz mentioned this pull request Feb 25, 2021

[2.8.x] Calling setScalaVersionFromSbtVersion in test-scripted is totally useless #10720

Closed

ignasi35 reviewed Feb 25, 2021

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

ignasi35 reviewed Feb 25, 2021

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

mkurz commented Feb 25, 2021

View reviewed changes

.travis.yml Show resolved Hide resolved

mkurz mentioned this pull request Feb 25, 2021

[2.8.x] Run scripted tests on sbt 1.4.x + refactoring #10723

Merged

mkurz force-pushed the sbt14_refactoring branch from 7acb9b2 to 8ab83b3 Compare March 5, 2021 09:41

mkurz mentioned this pull request Mar 5, 2021

Fix sourcePositionMappers added by Play not getting called sbt/sbt#6352

Merged

mkurz added 5 commits March 9, 2021 19:10

Run scripted tests on sbt 1.4.x/1.5.x + refactoring

d6b6bfc

No need for all those test scripts

8b7d9ef

-Yno-adapted-args got removed in Scala 2.13

501bd06

-Xfuture not available anymore

61fc472

Some scripted tests were not yet Scala 2.13 compatible

7db001a

mkurz force-pushed the sbt14_refactoring branch from 8ab83b3 to 7db001a Compare March 9, 2021 18:11

mkurz added 4 commits March 9, 2021 20:24

Remove sbt 1.5.0-M2

d7e7a0d

sbt 1.4.8

4e0f08e

Make scripted test "multiproject" sbt 1.4+ compatible

c60c5fa

Scripted test "play-position-mapper" has to (again) use extraLoggers …

58d8448

…for sbt 1.4+

Increase timeouts, fixes playframework#10742

523973e

mkurz mentioned this pull request Mar 9, 2021

Failed: Injected component fails (scripted tests) #10742

Closed

Remove sbt 1.2 and enable cron jobs

dfd8409

mkurz requested a review from ignasi35 March 9, 2021 23:33

mkurz added 4 commits March 10, 2021 17:39

sbt 1.4.9

d8551c4

No need for Future { } in makeRequestAndRecordResponseBody

8cd3f1c

More timeouts

09dd00c

No future -> no timeouts

f85e4a5

mkurz force-pushed the sbt14_refactoring branch from b67a3eb to f85e4a5 Compare March 11, 2021 10:46

mkurz removed the request for review from ignasi35 March 11, 2021 15:04

mkurz commented Mar 11, 2021

View reviewed changes

dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/dev-mode-compile-and-config-error-source/test Show resolved Hide resolved

mkurz commented Mar 11, 2021

View reviewed changes

dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/dev-mode-compile-and-config-error-source/test Show resolved Hide resolved

mkurz commented Mar 11, 2021

View reviewed changes

dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/show-versions/test Show resolved Hide resolved

mkurz commented Mar 11, 2021

View reviewed changes

mkurz removed the status:block-merge label Mar 11, 2021

ignasi35 approved these changes Mar 12, 2021

View reviewed changes

Remove obsolete comment

a91e71f

mkurz added the status:merge-when-green label Mar 13, 2021

mergify bot merged commit 8f7021d into playframework:master Mar 13, 2021

mergify bot removed the status:merge-when-green label Mar 13, 2021

mkurz deleted the sbt14_refactoring branch March 13, 2021 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run scripted tests on sbt 1.4.x + refactoring #10655

Run scripted tests on sbt 1.4.x + refactoring #10655

mkurz commented Jan 22, 2021 •

edited

Loading

mkurz commented Jan 27, 2021 •

edited

Loading

mkurz commented Feb 25, 2021

mkurz commented Mar 2, 2021

mkurz commented Mar 3, 2021 •

edited

Loading

mkurz commented Mar 5, 2021 •

edited

Loading

mkurz Mar 11, 2021 •

edited

Loading

mkurz Mar 11, 2021

mkurz Mar 11, 2021

mkurz Mar 11, 2021 •

edited

Loading

mkurz Mar 11, 2021 •

edited

Loading

mkurz Mar 11, 2021

mkurz Mar 11, 2021

mkurz Mar 11, 2021

mkurz commented Mar 11, 2021

ignasi35 left a comment

ignasi35 Mar 12, 2021

ignasi35 Mar 12, 2021

ignasi35 Mar 12, 2021

ignasi35 Mar 12, 2021

ignasi35 Mar 12, 2021

ignasi35 Mar 12, 2021

mkurz commented Mar 13, 2021

	val compileResult: Either[Incomplete, CompileSuccess] = for {
	analysis <- reloadCompile().toEither.right
	classpath <- classpath().toEither.right
	} yield CompileSuccess(sourceMap(analysis), classpath.files)
	compileResult.left.map(inc => CompileFailure(taskFailureHandler(inc, streams()))).merge

Run scripted tests on sbt 1.4.x + refactoring #10655

Run scripted tests on sbt 1.4.x + refactoring #10655

Conversation

mkurz commented Jan 22, 2021 • edited Loading

mkurz commented Jan 27, 2021 • edited Loading

mkurz commented Feb 25, 2021

mkurz commented Mar 2, 2021

mkurz commented Mar 3, 2021 • edited Loading

mkurz commented Mar 5, 2021 • edited Loading

mkurz Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurz Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

mkurz Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurz commented Mar 11, 2021

ignasi35 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurz commented Mar 13, 2021

mkurz commented Jan 22, 2021 •

edited

Loading

mkurz commented Jan 27, 2021 •

edited

Loading

mkurz commented Mar 3, 2021 •

edited

Loading

mkurz commented Mar 5, 2021 •

edited

Loading

mkurz Mar 11, 2021 •

edited

Loading

mkurz Mar 11, 2021 •

edited

Loading

mkurz Mar 11, 2021 •

edited

Loading