Pipeline tests simplification #1348
Conversation
Configure Renovate
| - name: run tests | ||
| env: | ||
| CURRENT_INDEX: ${{ matrix.current_index }} | ||
| NUMBER_OF_JOBS: ${{ matrix.number_of_jobs }} | ||
| run: | | ||
|
|
||
| # - find all tests |
There was a problem hiding this comment.
the idea of this PR is in these lines, I'll try to explain.
In this removed code, we were trying to find out what tests is this part of the matrix supposed to run. For example, we find out that there are 100 tests to run and there are 10 instances where these are supposed to run, so split 10 tests per instance.
In order to do this, the very first thing we needed is find out the fully qualified class names of the tests that are supposed to run. Why? Because we do not simply call mvn clean install, but instead mvn clean install -DtestsToRun=test1,test2.
This is not trivial. We can not simply look into a class and see if there is a @Test there: it could be abstract, it could be nested, it could parametrized, it could have cases that we are not even aware of (potentially what if junit-6 add one more types?). We were trying our best (with lots of headache) to find the correct test here, but this was not a good idea in general.
Instead, I am proposing a different approach. Let's use junit itself to tell us what these class names are. For that:
- I created an extension:
class DisabledTestsCondition implements ExecutionCondition - enabled it via:
junit.jupiter.extensions.autodetection.enabled=true - enabled it in
META-INF/services(in our internalspring-cloud-kubernetes-test-support), via:org.springframework.cloud.kubernetes.tests.commons.junit_extension.DisabledTestsCondition. - added the
spring-cloud-kubernetes-test-supportto all modules that have tests
Changes in the pipeline:
- first, find the tests, by doing:
./mvnw clean test integration-test -Dskip.build.image=true -Dspring.cloud.k8s.skip.tests=true \
-Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=error \
-T 1C > /tmp/tests.txt
Notice the -Dspring.cloud.k8s.skip.tests=true. In the extension, if this flag is true:
if ("true".equals(System.getProperty("spring.cloud.k8s.skip.tests"))) {
System.out.println("spring.cloud.k8s.test.to.run -> " + extensionContext.getRequiredTestClass().getName());
return ConditionEvaluationResult.disabled("");
}
that is:
- I log the class name
- skip this test class
Next, I upload these test class names in a file: /tmp/tests.txt
- Then, in each matrix step, I download the test names. Do a little bit of grep and parsing via awk to find the actual fully qualified class name, and the rest of the code remains the same.
There was a problem hiding this comment.
There are a few benefits. At least now, we are sure that we get all the tests (I double checked this by doing diffs locally, I can go into the details if needed how I have done that, just let me know). We rely on the junit engine itself to tell us what the tests are, which of course means we will get the correct tests this time.
The downside is that finding tests in this manner takes more time, around 5 minutes more, and 7/8 minutes more per entire run of the pipeline. But I am all in for correctness here and far easier maintenance. This will get even better in my next PR, where I will split the tests in a far more balanced way, so we might not even notice increased time here compared to all the runs until now.
|
@ryanjbaxter this is ready to be looked at too. I hope it will make sense why I think this is needed. |
|
|
||
| /** | ||
| * This is mainly needed for our pipeline, to get the test classes names. | ||
| * |
There was a problem hiding this comment.
Would be useful to provide an example of how to run it
| public class DisabledTestsCondition implements ExecutionCondition { | ||
|
|
||
| @Override | ||
| public ConditionEvaluationResult evaluateExecutionCondition(ExtensionContext extensionContext) { |
There was a problem hiding this comment.
Does this get executed for each test class? What does the resulting output look like?
I assume it looks like
spring.cloud.k8s.test.to.run -> foo.class
spring.cloud.k8s.test.to.run -> bar.class
There was a problem hiding this comment.
yes, this is executed for each test class. and the output is indeed the way you showed.
.github/workflows/maven.yaml
Outdated
| @@ -37,6 +37,9 @@ jobs: | |||
| - name: build with skip tests and skip images | |||
| run: ./mvnw -T 1C -s .settings.xml clean install -U -DskipTests -Dskip.build.image=true | |||
|
|
|||
| - name: find all tests | |||
There was a problem hiding this comment.
Is it necessary to run this separately? Can we combine it with the steps above where we skip running the tests and but find a list of tests we need to run?
There was a problem hiding this comment.
excellent suggestion! I don't know how I missed this :) thank you
| @@ -0,0 +1,20 @@ | |||
| name: find all tests | |||
There was a problem hiding this comment.
Maybe its best to rename this now as its purpose is to compile the code and find all tests? I image by combining these two steps we save sometime?
There was a problem hiding this comment.
right, it was too late for me to do it yesterday... and yes, it saved a few good minutes!
|
do you think there is more to be done here? |
No description provided.