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

Improve finding tests (1) #1420

Merged
merged 10 commits into from
Aug 30, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,26 @@ description: maven-build-with-dry-run-for-tests
runs:
using: "composite"
steps:
- name: maven-build-with-dry-run-for-tests

- name: run 'package' on the project
shell: bash
run: |
./mvnw install -B \
-Dskip.build.image=true \
-DskipTests -DskipITs \
-T 1C -q

- name: find all classpath entries
shell: bash
run: |
./mvnw -q exec:exec -Dexec.classpathScope="test" -Dexec.executable="echo" -Dexec.args="%classpath" | tr : "\n" > /tmp/deps.txt

- name: find all tests
shell: bash
run: |
# find all the tests that are supposed to be run, but don't actually run them.
Copy link
Contributor Author

@wind57 wind57 Aug 25, 2023

Choose a reason for hiding this comment

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

we have a series of problems that are waiting to pop-up and I am fixing them here, granted I created them via various changes.

For example this change: ./mvnw test -B -Dskip.build.image=true. It used to be ./mvnw clean install -B -Dskip.build.image=true. So test instead of clean install. This will cause three problems:

  • The first one can already be seen here. TL;DR : when we re-name things, instead of compiling with these new changes, we will try to compile with the ones from cache. Since there were renames - compilation will fail. As such, we really need install here.

  • The second one is that with this test goal, we will not find any integration tests (integration-test goal was take when we had install). And this is a serious problem. This means that our cache will never contain times for integration tests, which in turn means that at some point in time, the last step of some matrix will need to run all integration tests - and it will fail since there are too many of them. I already "caught" this issue in a build here.

  • The third one has to do with the way we find tests without running them, I will comment more under the appropriate files.

# this is achieved via: 'spring.cloud.k8s.skip.tests=true' in DisabledTestsCondition
./mvnw test -B -Dskip.build.image=true -Dspring.cloud.k8s.skip.tests=true \
-Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=error \
-T 1C -q > /tmp/tests.txt
cd spring-cloud-kubernetes-test-support
.././mvnw -q exec:java -Dexec.mainClass="org.springframework.cloud.kubernetes.tests.discovery.TestsDiscovery" > /tmp/tests.txt
cd ..

- name: show result
if: always()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ runs:
uses: actions/cache/restore@v3
with:
path: /tmp/sorted.txt
key: ${{ runner.os }}-spring-cloud-kubernetes-existing-test-times-cache-${{ github.run_id }}
restore-keys: ${{ runner.os }}-spring-cloud-kubernetes-existing-test-times-cache-
key: ${{ runner.os }}-spring-cloud-k8s-existing-test-times-cache-${{ github.run_id }}
restore-keys: ${{ runner.os }}-spring-cloud-k8s-existing-test-times-cache-

- name: show cached test times
shell: bash
Expand Down Expand Up @@ -138,7 +138,7 @@ runs:
# the previous cache will contain more tests then we currently have, so some
# matrix instances will have no tests to run
if [[ "$sum" -eq "0" ]]; then
echo "no tests to run in current index, most probably this PR removed some tests"
echo "no tests (with known times) to run in current index"
tests_to_run_in_current_index='none'
fi

Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/composites/test-times/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,12 @@ runs:

# save with the current run_id, but restore it without it. This means two things:
# 1) if we re-run, cache will be available
# 2) if there is a new run, we restore as '${{ runner.os }}-spring-cloud-kubernetes-existing-test-times-cache-'
# 2) if there is a new run, we restore as '${{ runner.os }}-spring-cloud-k8s-existing-test-times-cache-'
# meaning there could be many of them already present and this is not an exact match
# github in this case will pick up the latest one, exactly what we want.
- name: save test times in cache
uses: actions/cache/save@v3
if: env.BASE_BRANCH != '2.1.x'
with:
path: /tmp/sorted.txt
key: ${{ runner.os }}-spring-cloud-kubernetes-existing-test-times-cache-${{ github.run_id }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

with the change from install to test, I messed-up the cache. There are other ways to fix it, but this is the least painful one

key: ${{ runner.os }}-spring-cloud-k8s-existing-test-times-cache-${{ github.run_id }}
4 changes: 2 additions & 2 deletions .github/workflows/maven.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ jobs:
uses: actions/cache/restore@v3
with:
path: /tmp/sorted.txt
key: ${{ runner.os }}-spring-cloud-kubernetes-existing-test-times-cache-${{ github.run_id }}
restore-keys: ${{ runner.os }}-spring-cloud-kubernetes-existing-test-times-cache-
key: ${{ runner.os }}-spring-cloud-k8s-existing-test-times-cache-${{ github.run_id }}
restore-keys: ${{ runner.os }}-spring-cloud-k8s-existing-test-times-cache-

- name: check test times cache exists
id: check_files
Expand Down
5 changes: 0 additions & 5 deletions spring-cloud-kubernetes-client-discovery/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,6 @@
<artifactId>spring-boot-starter-test</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-web</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
/**
* @author wind57
*/
public class AbstractKubernetesProfileEnvironmentPostProcessorTest {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is an interesting one too. Now that we use the proper way to find tests, junit-5 finds this one. On the other hand because the name in the test contains Abstract, surefire skips it ([DEBUG] Resolved included and excluded patterns: org.springframework.cloud.kubernetes.commons.profile.AbstractKubernetesProfileEnvironmentPostProcessorTest, !**/Abstract*.java), as such our pipeline fails because of it. Rename it to fix this problem...

public class KubernetesProfileEnvironmentPostProcessorTest {

private static final String FOUNT_IT = "foundIt";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,6 @@
<artifactId>spring-rabbit-test</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,6 @@
<artifactId>spring-boot-starter-test</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
5 changes: 0 additions & 5 deletions spring-cloud-kubernetes-fabric8-config/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,6 @@
<artifactId>spring-boot-starter-test</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.springframework.cloud</groupId>
<artifactId>spring-cloud-test-support</artifactId>
Expand Down
6 changes: 0 additions & 6 deletions spring-cloud-kubernetes-fabric8-discovery/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,6 @@
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-web</artifactId>
Expand Down
28 changes: 27 additions & 1 deletion spring-cloud-kubernetes-test-support/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,15 @@

<properties>
<awaitility.version>4.0.3</awaitility.version>
<testcontainers.version>1.18.0</testcontainers.version>
<testcontainers.version>1.19.0</testcontainers.version>
</properties>
<dependencies>

<dependency>
<groupId>org.junit.platform</groupId>
<artifactId>junit-platform-launcher</artifactId>
</dependency>

<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-test</artifactId>
Expand Down Expand Up @@ -43,4 +49,24 @@

</dependencies>

<build>
<plugins>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>exec-maven-plugin</artifactId>
<version>3.1.0</version>
<executions>
<execution>
<goals>
<goal>java</goal>
</goals>
</execution>
</executions>
<configuration>
Copy link
Contributor

Choose a reason for hiding this comment

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

@wind57 I think we need to skip this unless a specific profile is activated. In other words we should only due this is we are running on GitHub actions. If we don't generate the tmp file before running the build, the build will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah! indeed, I did not think about this. PR is out

<mainClass>org.springframework.cloud.kubernetes.tests.discovery.TestsDiscovery</mainClass>
</configuration>
</plugin>
</plugins>
</build>

</project>
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
import org.springframework.util.StringUtils;

import static org.awaitility.Awaitility.await;
import static org.junit.Assert.fail;
import static org.junit.jupiter.api.Assertions.fail;

/**
* @author Ryan Baxter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
import org.springframework.cloud.kubernetes.integration.tests.commons.Phase;

import static org.awaitility.Awaitility.await;
import static org.junit.Assert.fail;
import static org.junit.jupiter.api.Assertions.fail;
import static org.springframework.cloud.kubernetes.integration.tests.commons.Commons.loadImage;
import static org.springframework.cloud.kubernetes.integration.tests.commons.Commons.pomVersion;
import static org.springframework.cloud.kubernetes.integration.tests.commons.Commons.pullImage;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
import org.springframework.cloud.kubernetes.integration.tests.commons.Phase;

import static org.awaitility.Awaitility.await;
import static org.junit.Assert.fail;
import static org.junit.jupiter.api.Assertions.fail;
import static org.springframework.cloud.kubernetes.integration.tests.commons.Commons.loadImage;
import static org.springframework.cloud.kubernetes.integration.tests.commons.Commons.pomVersion;
import static org.springframework.cloud.kubernetes.integration.tests.commons.Commons.pullImage;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
*
* @author wind57
*/
@Deprecated
public class DisabledTestsCondition implements ExecutionCondition {

private static final boolean SKIP_RUNNING_TESTS = "true".equals(System.getProperty("spring.cloud.k8s.skip.tests"));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* Copyright 2013-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.cloud.kubernetes.tests.discovery;

import java.io.File;
import java.net.URI;
import java.net.URL;
import java.net.URLClassLoader;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.junit.platform.engine.discovery.DiscoverySelectors;
import org.junit.platform.launcher.Launcher;
import org.junit.platform.launcher.LauncherDiscoveryRequest;
import org.junit.platform.launcher.TestIdentifier;
import org.junit.platform.launcher.TestPlan;
import org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder;
import org.junit.platform.launcher.core.LauncherFactory;

/**
* @author wind57
*/
public class TestsDiscovery {

public static void main(String[] args) throws Exception {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it turns out I was wrong, here is the story.

We need to find what tests we have without running them. Initially, of course, I went to stackoverflow and the solution that we currently have seemed reasonable. But then, I read a bit of junit-5 documentation and found this, which clearly says:

Having test discovery as a dedicated feature of the platform itself frees IDEs and build tools from most of the difficulties they had to go through to identify test classes and test methods in previous versions of JUnit.

Exactly what we want to achieve: a dedicated feature/api to find out what tests we have.
The code is a bit involved because what we want to do is find out what tests we have from "outside" of the JVM. Granted, the same thing is done in ConsoleLuncher in junit-5 itself, and this is where my inspiration come from.


How this works is you tell junit where to find classes + classpath to use when searching for those. That is why before this class is called, I have two pipeline stepts:

    - name: run 'install' on the project
      shell: bash
      run: |
        ./mvnw install -B  \
              -Dskip.build.image=true \
              -DskipTests -DskipITs \
              -T 1C -q

        - name: find all classpath entries
           shell: bash
           run: |
           ./mvnw -q exec:exec -Dexec.classpathScope="test" -Dexec.executable="echo" -Dexec.args="%classpath" | tr : "\n" > /tmp/deps.txt

the first one takes care to create target/classes and target/test-classes; and the second one just gets a huge list of Strings that is the projects classpath. This info is fed to proper junit api and it will tell you all the tests you have. This is actually awesome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the added benefit is that this is even faster then the previous solution

List<String> targetClasses = entireClasspath().stream().filter(x -> x.contains("target/classes")).toList();
List<String> targetTestClasses = targetClasses.stream().map(x -> x.replace("classes", "test-classes")).toList();
List<String> jars = entireClasspath().stream().filter(x -> x.contains(".jar")).toList();

List<URL> urls = Stream.of(targetClasses, targetTestClasses, jars).flatMap(List::stream)
.map(x -> toURL(new File(x).toPath().toUri())).toList();

Set<Path> paths = Stream.of(targetClasses, targetTestClasses, jars).flatMap(List::stream).map(Paths::get)
.collect(Collectors.toSet());

replaceClassloader(urls);

LauncherDiscoveryRequest request = LauncherDiscoveryRequestBuilder.request()
.selectors(DiscoverySelectors.selectClasspathRoots(paths)).build();

Launcher launcher = LauncherFactory.openSession().getLauncher();
TestPlan testPlan = launcher.discover(request);
testPlan.getRoots().stream().flatMap(x -> testPlan.getChildren(x).stream())
.map(TestIdentifier::getLegacyReportingName).sorted().forEach(test -> {
System.out.println("spring.cloud.k8s.test.to.run -> " + test);
});
}

private static void replaceClassloader(List<URL> classpathURLs) {
ClassLoader parentClassLoader = Thread.currentThread().getContextClassLoader();
URLClassLoader classLoader = URLClassLoader.newInstance(classpathURLs.toArray(new URL[0]), parentClassLoader);
Thread.currentThread().setContextClassLoader(classLoader);
}

// /tmp/deps.txt are created by the pipeline
private static List<String> entireClasspath() throws Exception {
return Files.lines(Paths.get("/tmp/deps.txt")).distinct().collect(Collectors.toList());
}

private static URL toURL(URI uri) {
try {
return uri.toURL();
}
catch (Exception e) {
throw new RuntimeException(e);
}
}

}

This file was deleted.

1 change: 1 addition & 0 deletions src/checkstyle/checkstyle-suppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,5 @@
<suppress files=".*DiscoveryServerApplication\.java" checks="HideUtilityClassConstructor"/>
<suppress files=".*KubernetesConfigServerApplication\.java" checks="HideUtilityClassConstructor"/>
<suppress files=".*Fabric8ConfigContext\.java" checks="WhitespaceAround" />
<suppress files=".*TestsDiscovery\.java" checks="HideUtilityClassConstructor" />
</suppressions>