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

Update sbt-sourcegraph and re-enable SBT build tool tests #636

Merged
merged 4 commits into from
Aug 16, 2023

Conversation

keynmol
Copy link
Contributor

@keynmol keynmol commented Aug 16, 2023

Now that we can run auto-indexing for SBT and support Scala 3, it's time to re-enable the tests

Test plan

  • Re-enabled disabled tests
  • Expanded testing matrix

@keynmol
Copy link
Contributor Author

keynmol commented Aug 16, 2023

@olafurpg

Note that I have some changes locally to be able to run build tool tests in parallel, but it seems to suffer from a concurrency bug, where MUnit prints all suite names first and then all test results
CleanShot 2023-08-16 at 13 58 26@2x

FTR, the diff is

diff --git a/build.sbt b/build.sbt
index 4b2a864..52066b7 100644
--- a/build.sbt
+++ b/build.sbt
@@ -509,7 +509,8 @@ lazy val buildTools = project
       Map(
         "SCIP_JAVA_CLI" -> ((cli / pack).value / "bin" / "scip-java").toString
       ),
-    Test / fork := true
+    Test / fork := true,
+    Test / testForkedParallel := true
   )
   .dependsOn(agent, unit)
 
diff --git a/tests/buildTools/src/test/scala/tests/BaseBuildToolSuite.scala b/tests/buildTools/src/test/scala/tests/BaseBuildToolSuite.scala
index 80fe64a..d4401fb 100644
--- a/tests/buildTools/src/test/scala/tests/BaseBuildToolSuite.scala
+++ b/tests/buildTools/src/test/scala/tests/BaseBuildToolSuite.scala
@@ -21,6 +21,7 @@ import os.Shellable
 object Java8Only extends munit.Tag("Java8Only")
 
 abstract class BaseBuildToolSuite extends MopedSuite(ScipJava.app) {
+  self =>
   override def environmentVariables: Map[String, String] = sys.env
 
   def tags = List.empty[Tag]
@@ -49,8 +50,15 @@ abstract class BaseBuildToolSuite extends MopedSuite(ScipJava.app) {
   // NOTE(olafur): workaround for https://github.com/scalameta/moped/issues/18
   override val temporaryDirectory: DirectoryFixture =
     new DirectoryFixture {
-      private val path = BuildInfo.temporaryDirectory.toPath
-      override def apply(): Path = path
+      private val path = BuildInfo
+        .temporaryDirectory
+        .toPath
+        .resolve(self.getClass().getSimpleName())
+
+      override def apply(): Path = {
+        Files.createDirectories(path)
+        path
+      }
       override def beforeEach(context: BeforeEach): Unit = {
         DeleteVisitor.deleteRecursively(path)
       }

@olafurpg
Copy link
Member

@keynmol what happens if you remove the -b option here? IIRC, MUnit supports using the sbt test logger, which groups by suite https://github.com/sourcegraph/scip-java/blob/main/build.sbt#L571

@keynmol
Copy link
Contributor Author

keynmol commented Aug 16, 2023

That seems to have helped - I made buildTools the only module running in parallel

private val path = BuildInfo
.temporaryDirectory
.toPath
.resolve(self.getClass().getSimpleName())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing this so that individual suites don't clobber each other (otherwise you get issues with parallel folder deletion and all that).

Hacky but works for now.

GHA doesn't have the oomph to give us any benefit there
@keynmol
Copy link
Contributor Author

keynmol commented Aug 16, 2023

Ok, I made the tests sequential again on CI only - GHA workers have such tiny vCPUs and barely two of them, that it's not worth the increased GC overhead

@keynmol keynmol requested a review from olafurpg August 16, 2023 14:04
@keynmol keynmol enabled auto-merge (squash) August 16, 2023 14:10
@@ -509,7 +509,10 @@ lazy val buildTools = project
Map(
"SCIP_JAVA_CLI" -> ((cli / pack).value / "bin" / "scip-java").toString
),
Test / fork := true
Test / fork := true,
// Our CI set up is a couple of measly vCPUs so parallelising tests there makes
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 probably get access to more powerful buildkite machines if we want to. I never looked into it.

@keynmol keynmol merged commit 6f17cbe into main Aug 16, 2023
11 checks passed
@keynmol keynmol deleted the re-enable-sbt-tests branch August 16, 2023 14:15
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.

None yet

2 participants