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

Conversation

wind57
Copy link
Contributor

@wind57 wind57 commented Aug 25, 2023

No description provided.

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.

*/
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

# 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

@@ -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...

@wind57 wind57 marked this pull request as ready for review August 26, 2023 08:47
@wind57
Copy link
Contributor Author

wind57 commented Aug 26, 2023

@ryanjbaxter ready to be looked at, I left comments on all the important things here. thank you

@wind57 wind57 changed the title Improve finding tests Improve finding tests (1) Aug 27, 2023
@ryanjbaxter ryanjbaxter added this to the 3.0.5 milestone Aug 30, 2023
@ryanjbaxter ryanjbaxter merged commit 078fba1 into spring-cloud:3.0.x Aug 30, 2023
24 checks passed
</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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants