[Feature]: Improved testing (#194)

.github/gradle/gradle.gradle

@@ -1,41 +1,119 @@
+import groovy.json.JsonBuilder
+import groovy.transform.CompileStatic
+
 if (!project.hasProperty('githubCiTesting') && System.env['GITHUB_OUTPUT'] == null) {
     // If the property is not set and we are not running in the CI environment then we will do nothing
     return
 }
 
-static def outputUnitTest(Project project, Task test) {
-    project.getLogger().lifecycle("<< TEST >>${test.path}")
+static Map<String, String> removeCommonPrefix(List<String> list) {
+    if (list.size() == 0) {
+        return [:]
+    }
+
+    def result = new HashMap<String, String>()
+
+    if (list.unique(false).size() == 1) {
+        def sections = list[0].split("\\.")
+        result.put(list[0], sections.last())
+        return result
+    }
+
+    Collections.sort(list, { String a, String b -> a.length() - b.length() })
+    def max = list[0].length()
+    def prefix = ""
+
+    for(int index = 0; index < max; index++) {
+        def currentChar = list[0][index]
+        for (String item : list) {
+            if (item[index] != currentChar) {
+                for (final def entry in list) {
+                    result.put(entry, entry.substring(prefix.length()))
+                }
+                return result
+            }
+        }
+
+        prefix += currentChar
+    }
+
+    throw new IllegalArgumentException("Could not remove common prefix: ${list}")
 }
 
-static def outputClassTest(Project project, Task test, String className) {
-    project.getLogger().lifecycle("<< TEST >>${test.path} --tests \"${className}\"")
+static String createDisplayNameSuffix(String path, String filter) {
+    //The path starts with a : and then the project name:
+    path = path.substring(path.indexOf(":") + 1)
+
+    //Now split the path into parts
+    def parts = path.split(":")
+    def capitalizedParts = parts.collect { it.capitalize() }
+
+    //Combine the parts with ->
+    path = capitalizedParts.join(" -> ")
+
+    if (filter === null) {
+        return path
+    }
+
+    //Next the filter has its common prefix stripped, is however still in domain name form
+    def filterParts = filter.split("\\.")
+    def capitalizedFilterParts = filterParts.collect { it.capitalize() }
+    filter = capitalizedFilterParts.join("/")
+
+    return "${path} (${filter})"
+}
+
+static List<TestRun> createTestRuns(Task it, List<String> testClasses) {
+    def testRuns = []
+    if (testClasses.size() == 0) {
+        testRuns.add(new TestRun(displayName: "Test - ${createDisplayNameSuffix(it.path, null)}", path: it.path, filter: null))
+        return testRuns
+    }
+
+    def testClassesWithoutCommonPrefix = removeCommonPrefix(testClasses)
+    testClassesWithoutCommonPrefix.forEach { className, displayName ->
+        testRuns.add(new TestRun(displayName: "Test - ${createDisplayNameSuffix(it.path, displayName)}", path: it.path, filter: className))
+    }
+
+    return testRuns
+}
+
+@CompileStatic
+class TestRun {
+    String displayName
+    String path
+    String filter
 }
 
 subprojects.forEach { Project subProject ->
     subProject.tasks.register('determineTests') { Task it ->
+        def tests = []
+
         it.group = 'infrastructure'
         it.doLast {
             subProject.tasks.withType(Test).forEach { Task test ->
                 def testSourceSetCandidate = test.extensions.findByName('test-source-set')
                 if (testSourceSetCandidate != null) {
                     SourceSet testSourceSet = testSourceSetCandidate as SourceSet
 
+                    def testClasses = []
+
                     testSourceSet.java.srcDirs
                             .collect { File file ->
                                 subProject.fileTree(file.parentFile)
                             }
                             .collect { FileTree fileTree ->
-                                return fileTree.matching {
-                                    include '**/*Test.java'
-                                    include '**/*Tests.java'
-                                    include '**/*Test.groovy'
-                                    include '**/*Tests.groovy'
+                                return fileTree.matching { PatternFilterable pattern ->
+                                    pattern.include '**/*Test.java'
+                                    pattern.include '**/*Tests.java'
+                                    pattern.include '**/*Test.groovy'
+                                    pattern.include '**/*Tests.groovy'
                                 }
                             }
                             .forEach {
                                 it.visit { FileVisitDetails details ->
                                     if (details.isDirectory())
-                                        return;
+                                        return
 
                                     String className = details.relativePath.pathString
                                             .replace("groovy/", "")
@@ -44,14 +122,19 @@ subprojects.forEach { Project subProject ->
                                             .replace(".java", "")
                                             .replace("/", ".")
 
-                                    outputClassTest(subProject, test, className)
+                                    testClasses.add(className)
                                 }
                             }
 
+                    tests.addAll(createTestRuns(test, testClasses))
                 } else {
-                    outputUnitTest(subProject, test)
+                    tests.addAll(createTestRuns(test, new ArrayList<String>()))
                 }
             }
+
+            if (!tests.isEmpty()) {
+                project.getLogger().lifecycle("<< TEST >>${new JsonBuilder(tests)}")
+            }
         }
     }
 }

.github/scripts/collect-tests.sh

@@ -1,7 +1,6 @@
 #!/usr/bin/env bash
 
-./gradlew determineTests | grep -e '<< TEST >>' | sed -e 's/<< TEST >>//g' > ./tasks
-TESTS=$(cat tasks | jq --raw-input . | jq --compact-output --slurp .)
+TESTS=$(./gradlew determineTests | grep -e '<< TEST >>' | sed -e 's/<< TEST >>//g' | jq -s 'add' | jq -c .)
 # Check if the GITHUB_OUTPUT is set
 if [ -z "$GITHUB_OUTPUT" ]; then
   # We do not have github output, then use the set output command

.github/workflows/test-prs.yml

@@ -9,6 +9,10 @@ on:
       - ready_for_review
       - reopened
 
+concurrency:
+  group: ci-${{ github.head_ref || github.run_id }}
+  cancel-in-progress: true
+
 jobs:
   setup:
     name: Setup
@@ -62,13 +66,15 @@ jobs:
         run: ./gradlew --info -s -x assemble
 
   test:
-    name: Test
-    runs-on: ubuntu-latest
+    name: "${{ matrix.test.displayName }} (${{ matrix.os }})"
+    runs-on: "${{ matrix.os }}-latest"
     needs: setup
     strategy:
+      max-parallel: 15
       fail-fast: false
       matrix:
         test: ${{ fromJSON(needs.setup.outputs.tests-to-run) }}
+        os: [ubuntu, windows, macos]
     steps:
       - name: Checkout repository
         uses: actions/checkout@v4
@@ -83,45 +89,77 @@ jobs:
           distribution: 'microsoft'
 
       - name: Setup Gradle
-        uses: gradle/gradle-build-action@v3
+        uses: gradle/actions/setup-gradle@v3
 
       - name: Test
-        run: ./gradlew --info -s ${{ matrix.test }}
+        if: ${{ matrix.test.filter != null }}
+        run: ./gradlew --info -s ${{ matrix.test.path }} --tests "${{ matrix.test.filter }}"
+
+      - name: Test
+        if: ${{ matrix.test.filter == null }}
+        run: ./gradlew --info -s ${{ matrix.test.path }}
 
       # Always upload test results
       - name: Merge Test Reports
         if: success() || failure()
         run: npx junit-report-merger junit.xml "**/TEST-*.xml"
 
       - name: Format test run name as artifact name
-        id: format-artifact-name
+        if: (success() || failure()) &&  runner.os == 'Windows'
+        id: format-artifact-name-windows
+        # Use the GITHUB_OUTPUT mechanic to set the output variable
+        run: |
+          # We need to remove all invalid characters from the test name:
+          # Invalid characters include:  Double quote ", Colon :, Less than <, Greater than >, Vertical bar |, Asterisk *, Question mark ?, Carriage return \r, Line feed \n, Backslash \, Forward slash /
+          $NAME = "${{ matrix.test.displayName }}" -replace '[":<>|*?\\/]', '-' -replace ' ', ''
+          
+          # Check if the GITHUB_OUTPUT is set
+          Write-Output "Determined name to be $NAME-${{ matrix.os }}"
+          if ([string]::IsNullOrEmpty($env:GITHUB_OUTPUT)) {
+            # We do not have github output, then use the set output command
+            Write-Output "::set-output name=artifact-name::$NAME-${{ matrix.os }}"
+            exit
+          }
+          Add-Content -Path $env:GITHUB_OUTPUT -Value "artifact-name=$NAME-${{ matrix.os }}"
+
+      - name: Format test run name as artifact name
+        if: (success() || failure()) &&  runner.os != 'Windows'
+        id: format-artifact-name-unix
         # Use the GITHUB_OUTPUT mechanic to set the output variable
         run: |
-          # We have two cases here, one with a gradle task path, there we replace the : with a - and strip the leading -
-          # The other case is complexer, again we replace the : with a - and strip the leading - but now we also remove the '  --tests ' part
-          # Remove the '"' from the string and replace the '.' with a '-' 
-          NAME=$(echo "${{ matrix.test }}" | sed 's/:/-/g' | sed 's/^-//' | sed 's/  --tests /-/g' | sed 's/"//g' | sed 's/\./-/g')         
+          # We need to remove all invalid characters from the test name:
+          # Invalid characters include:  Double quote ", Colon :, Less than <, Greater than >, Vertical bar |, Asterisk *, Question mark ?, Carriage return \r, Line feed \n, Backslash \, Forward slash /
+          NAME=$(echo "${{ matrix.test.displayName }}" | tr '":<>|*?\\/' '-' | tr -d ' ')          
           # Check if the GITHUB_OUTPUT is set
+          echo "Determined name to be $NAME-${{ matrix.os }}"
           if [ -z "$GITHUB_OUTPUT" ]; then
             # We do not have github output, then use the set output command
-            echo "::set-output name=artifact-name::$NAME"
+            echo "::set-output name=artifact-name::$NAME-${{ matrix.os }}"
             exit 0
           fi
-          echo "artifact-name=$NAME" >> "$GITHUB_OUTPUT"
+          echo "artifact-name=$NAME-${{ matrix.os }}" >> "$GITHUB_OUTPUT"
 
+      - uses: actions/upload-artifact@v4
+        if: (success() || failure()) &&  runner.os != 'Windows'
+        with:
+          if-no-files-found: ignore
+          name: test-results-${{ steps.format-artifact-name-unix.outputs.artifact-name }}
+          path: junit.xml
+          retention-days: 1
 
       - uses: actions/upload-artifact@v4
-        if: success() || failure()
+        if: (success() || failure()) &&  runner.os == 'Windows'
         with:
           if-no-files-found: ignore
-          name: test-results-${{ steps.format-artifact-name.outputs.artifact-name }}
+          name: test-results-${{ steps.format-artifact-name-windows.outputs.artifact-name }}
           path: junit.xml
           retention-days: 1
 
   process-test-data:
+    name: Process Test Data
     runs-on: ubuntu-latest
     needs: test
-    if: success() || failure()
+    if: always()
     steps:
       - uses: actions/checkout@v3
 
@@ -133,16 +171,29 @@ jobs:
 
       - name: Publish Test Report
         uses: mikepenz/action-junit-report@v4
-        if: success() || failure() # always run even if the previous step fails
+        if: always() # always run even if the previous step fails
         with:
           report_paths: '**/*.xml'
 
       - name: Merge Test Reports
-        if: success() || failure()
+        if: always()
         run: npx junit-report-merger junit.xml "**/*.xml"
 
       - uses: actions/upload-artifact@v4
-        if: success() || failure()
+        if: always()
         with:
           name: test-results
           path: junit.xml
+
+      - name: Failed build detection
+        uses: actions/github-script@v7
+        if: >-
+          ${{
+               contains(needs.*.result, 'failure')
+            || contains(needs.*.result, 'cancelled')
+            || contains(needs.*.result, 'skipped')
+          }}
+        with:
+          script: |
+            core.setFailed('Test build failure!')
+

build.gradle

@@ -163,6 +163,7 @@ subprojects.forEach { subProject ->
             spec.exclude group: 'org.codehaus.groovy'
         }
         evalSubProject.dependencies.functionalTestImplementation "net.neoforged.trainingwheels:gradle-functional:${project.trainingwheels_version}"
+        evalSubProject.dependencies.functionalTestImplementation project(':test-utils')
 
         //Configure the plugin metadata, so we can publish it.
         evalSubProject.gradlePlugin.plugins { NamedDomainObjectContainer<PluginDeclaration> plugins ->

common/src/main/java/net/neoforged/gradle/common/CommonPlugin.java

@@ -2,12 +2,12 @@
 
 import org.gradle.api.Plugin;
 import org.gradle.api.Project;
+import org.jetbrains.annotations.NotNull;
 
 public class CommonPlugin implements Plugin<Object> {
     @Override
-    public void apply(Object o) {
-        if (o instanceof Project) {
-            final Project project = (Project) o;
+    public void apply(@NotNull Object o) {
+        if (o instanceof Project project) {
             project.getPluginManager().apply(CommonProjectPlugin.class);
         } else {
             throw new IllegalArgumentException("CommonPlugin can only be applied to a project");

common/src/main/java/net/neoforged/gradle/common/CommonProjectPlugin.java

@@ -116,7 +116,7 @@ public void apply(Project project) {
 
         project.getRepositories().maven(e -> {
             e.setUrl(UrlConstants.MOJANG_MAVEN);
-            e.metadataSources(MavenArtifactRepository.MetadataSources::artifact);
+            e.metadataSources(MavenArtifactRepository.MetadataSources::mavenPom);
         });
 
         project.getExtensions().getByType(SourceSetContainer.class).configureEach(sourceSet -> {

common/src/main/java/net/neoforged/gradle/common/util/ToolUtilities.java

@@ -1,12 +1,10 @@
 package net.neoforged.gradle.common.util;
 
-import net.neoforged.gradle.dsl.common.extensions.repository.Repository;
 import net.neoforged.gradle.dsl.common.util.ConfigurationUtils;
 import net.neoforged.gradle.util.ModuleDependencyUtils;
 import org.gradle.api.Project;
 import org.gradle.api.artifacts.Dependency;
 import org.gradle.api.artifacts.ResolvedArtifact;
-import org.gradle.api.artifacts.repositories.ArtifactRepository;
 
 import java.io.File;
 import java.util.function.Supplier;
@@ -18,33 +16,30 @@ private ToolUtilities() {
     }
 
     public static File resolveTool(final Project project, final String tool) {
-        return resolveTool(project, () -> ConfigurationUtils.temporaryUnhandledConfiguration(
+        return resolveTool(() -> ConfigurationUtils.temporaryUnhandledConfiguration(
                 project.getConfigurations(),
                 "ToolLookupFor" + ModuleDependencyUtils.toConfigurationName(tool),
                 project.getDependencies().create(tool)
         ).getFiles().iterator().next());
     }
 
     public static ResolvedArtifact resolveToolArtifact(final Project project, final String tool) {
-        return resolveTool(project, () ->  ConfigurationUtils.temporaryUnhandledConfiguration(
+        return resolveTool(() ->  ConfigurationUtils.temporaryUnhandledConfiguration(
                 project.getConfigurations(),
                 "ToolLookupFor" + ModuleDependencyUtils.toConfigurationName(tool),
                 project.getDependencies().create(tool)
         ).getResolvedConfiguration().getResolvedArtifacts().iterator().next());
     }
 
     public static ResolvedArtifact resolveToolArtifact(final Project project, final Dependency tool) {
-        return resolveTool(project, () -> ConfigurationUtils.temporaryUnhandledConfiguration(
+        return resolveTool(() -> ConfigurationUtils.temporaryUnhandledConfiguration(
                 project.getConfigurations(),
                 "ToolLookupFor" + ModuleDependencyUtils.toConfigurationName(tool),
                 tool
         ).getResolvedConfiguration().getResolvedArtifacts().iterator().next());
     }
 
-    private static <T> T resolveTool(final Project project, final Supplier<T> searcher) {
-        //Grab the dynamic repository
-        final Repository repository = project.getExtensions().getByType(Repository.class);
-
+    private static <T> T resolveTool(final Supplier<T> searcher) {
         //Return the resolved artifact
         return searcher.get();
     }