Skip to content

Test: Split tests by tag#486

Draft
qligier wants to merge 10 commits into
mainfrom
feat/split-tests
Draft

Test: Split tests by tag#486
qligier wants to merge 10 commits into
mainfrom
feat/split-tests

Conversation

@qligier
Copy link
Copy Markdown
Member

@qligier qligier commented Mar 22, 2026

This is an attempt at optimizing the memory consumption of tests by grouping related tests, and running JUnit once per group.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces JUnit 5 tags to split the test suite into logical groups and updates the GitHub Actions Maven workflow to run each group in a separate Maven invocation, aiming to reduce peak memory usage during CI test runs.

Changes:

  • Added a TestTags constants class and annotated tests with @Tag(...) to group them (core/validation/gazelle/mapping).
  • Updated multiple matchbox-server test classes to use tags (and cleaned up some visibility/imports).
  • Changed .github/workflows/maven.yml to run Maven tests once per group instead of a single full verify.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
matchbox-server/src/test/java/ch/ahdis/matchbox/test/MatchboxApiR5onR4Test.java Adds JUnit tags to split tests into groups.
matchbox-server/src/test/java/ch/ahdis/matchbox/test/MatchboxApiR5Test.java Adds JUnit tags and removes some unused imports.
matchbox-server/src/test/java/ch/ahdis/matchbox/test/MatchboxApiR4Test.java Tags individual tests into validation/core/gazelle groups.
matchbox-server/src/test/java/ch/ahdis/matchbox/test/MatchboxApiR4BTest.java Adds JUnit tags to split tests into groups.
matchbox-server/src/test/java/ch/ahdis/matchbox/mapping/TransformTest.java Tags the whole class as mapping-related.
matchbox-server/src/test/java/ch/ahdis/matchbox/gazelle/GazelleApiR4Test.java Tags the whole class as gazelle-related.
matchbox-server/src/test/java/ch/ahdis/matchbox/TestTags.java Introduces shared tag constants for tests.
.github/workflows/maven.yml Runs Maven tests four times, one per group/tag.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/maven.yml Outdated
Comment on lines 26 to 32
- name: Run the core tests in Maven
timeout-minutes: 15
# We need a timeout here
env:
MAVEN_OPTS: -Xmx12g
run: mvn --batch-mode --no-transfer-progress --update-snapshots verify

- uses: actions/upload-artifact@v4
if: failure()
with:
name: testresults
path: |
./matchbox-server/target/surefire-reports
./matchbox-engine/target/surefire-reports
retention-days: 2
run: mvn --batch-mode --no-transfer-progress --update-snapshots -Dgroups='core' test

Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

The workflow now runs only the test phase. Previously it ran verify, which also executes any verify-bound checks (e.g., duplicate-finder) and can affect integration-test execution via failsafe. If those checks are still required in CI, switch back to verify (or add an explicit step to run the missing phases) while still filtering by tag/group.

Copilot uses AI. Check for mistakes.
Comment thread .github/workflows/maven.yml Outdated
# We need a timeout here
env:
MAVEN_OPTS: -Xmx12g
run: mvn --batch-mode --no-transfer-progress --update-snapshots -Dgroups='mapping' test
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

CI now runs Maven four times with -Dgroups=.... Only tests tagged into these groups will execute; currently the matchbox-engine module tests are untagged (e.g., matchbox-engine/src/test/java/ch/ahdis/matchbox/engine/tests/R4ValidationTests), so they would be skipped in all four runs. Consider either tagging engine tests too, adding an additional run for untagged tests, or restricting these grouped runs to the module(s) that actually use the tags.

Suggested change
run: mvn --batch-mode --no-transfer-progress --update-snapshots -Dgroups='mapping' test
run: mvn --batch-mode --no-transfer-progress --update-snapshots -Dgroups='mapping' test
- name: Run ungrouped tests in Maven
timeout-minutes: 15
# We need a timeout here
env:
MAVEN_OPTS: -Xmx12g
run: mvn --batch-mode --no-transfer-progress --update-snapshots test

Copilot uses AI. Check for mistakes.
Comment thread .github/workflows/maven.yml Outdated
# We need a timeout here
env:
MAVEN_OPTS: -Xmx12g
run: mvn --batch-mode --no-transfer-progress --update-snapshots -Dgroups='mapping' test
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

This workflow previously uploaded surefire reports and produced a test summary; those steps were removed. That makes CI failures harder to diagnose and reduces visibility into which tests failed. Consider re-adding artifact upload/test summary (potentially after each group run, or once at the end with if: always()).

Suggested change
run: mvn --batch-mode --no-transfer-progress --update-snapshots -Dgroups='mapping' test
run: mvn --batch-mode --no-transfer-progress --update-snapshots -Dgroups='mapping' test
- name: Upload surefire test reports
if: always()
uses: actions/upload-artifact@v4
with:
name: surefire-reports
path: |
**/target/surefire-reports/**
**/target/failsafe-reports/**
- name: Publish test summary
if: always()
uses: dorny/test-reporter@v1
with:
name: Maven tests
path: "**/target/surefire-reports/TEST-*.xml"
reporter: java-junit

Copilot uses AI. Check for mistakes.
Comment on lines +6 to +10
public class TestTags {
public static final String CORE = "core";
public static final String VALIDATION = "validation";
public static final String GAZELLE = "gazelle";
public static final String MAPPING = "mapping";
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

TestTags is a constants holder and shouldn’t be instantiable. Consider making the class final and adding a private constructor to prevent accidental construction/extension.

Suggested change
public class TestTags {
public static final String CORE = "core";
public static final String VALIDATION = "validation";
public static final String GAZELLE = "gazelle";
public static final String MAPPING = "mapping";
public final class TestTags {
public static final String CORE = "core";
public static final String VALIDATION = "validation";
public static final String GAZELLE = "gazelle";
public static final String MAPPING = "mapping";
private TestTags() {
// Prevent instantiation
}

Copilot uses AI. Check for mistakes.
@qligier qligier marked this pull request as draft April 14, 2026 09:32
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.

2 participants