Skip to content

Conversation

@duanemay
Copy link
Member

also rename jobs to make it easier to distinguish unit/int test runs

rename jobs to make it easier to distinguish unit/int test runs
Copilot AI review requested due to automatic review settings December 15, 2025 14:19
strehle
strehle previously approved these changes Dec 15, 2025
Copy link
Contributor

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 optimizes Gradle build commands in test scripts and improves GitHub workflow job naming for better clarity. The changes remove redundant Gradle tasks while maintaining correct build dependencies through Gradle's task dependency graph.

Key Changes:

  • Streamlined Gradle commands by removing redundant assemble tasks where compilation dependencies handle the requirements
  • Renamed workflow jobs from generic container-test-job to specific db-unit-test and db-integration-test for better identification
  • Added clean to integration test assembly step for consistency

Reviewed changes

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

File Description
scripts/unit_tests.sh Removed assemble from first Gradle invocation; compileTestJava dependency chain ensures main code compilation
scripts/integration_tests.sh Added clean to assembly command for consistency; removed redundant clean assemble from test compilation step that runs after initial assembly
.github/workflows/unit-tests.yml Renamed job from container-test-job to db-unit-test for clearer identification
.github/workflows/integration-tests.yml Renamed job from container-test-job to db-integration-test to distinguish from unit test job

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

Restore the ability to search for " failed" without these extra matches
Copy link
Contributor

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

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


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

Comment on lines +113 to +116
-DskipUaaAutoStart=true \
--no-daemon \
--no-configuration-cache \
compileTestJava
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The first compileTestJava command is missing the same memory configuration flags that are present in the second test execution command. This could lead to inconsistent behavior or compilation failures due to insufficient memory allocation. The command should include -Djava.security.egd, -Dorg.gradle.jvmargs, -Dorg.gradle.daemon.idletimeout, -Dorg.gradle.parallel, and -Dorg.gradle.workers.max flags for consistency with the integration test execution step and to prevent potential compilation issues.

Suggested change
-DskipUaaAutoStart=true \
--no-daemon \
--no-configuration-cache \
compileTestJava
-Djava.security.egd=file:/dev/./urandom \
-DskipUaaAutoStart=true \
-Dorg.gradle.jvmargs="-Dfile.encoding=utf8 -Xms64m -Xmx${gradle_test_heap} -XX:MaxMetaspaceSize=128m -XX:+UseG1GC -XX:MaxGCPauseMillis=100 -XX:ParallelGCThreads=2 -XX:CICompilerCount=2 -Djdk.lang.processReaperUseDefaultStackSize=true" \
-Dorg.gradle.daemon.idletimeout=300000 \
-Dorg.gradle.parallel=false \
-Dorg.gradle.workers.max=2 \
compileTestJava \
--no-watch-fs \
--no-daemon \
--no-configuration-cache \
--max-workers=2 \
--stacktrace \
--console=plain

Copilot uses AI. Check for mistakes.
-DskipUaaAutoStart=true \
--no-daemon \
--no-configuration-cache \
compileTestJava
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The clean task was removed from this command but is still present in line 32 of scripts/unit_tests.sh. This inconsistency means integration tests now skip the clean task while unit tests still run it. If this difference is intentional (perhaps because assemble on line 51 already handles cleanup), the behavior should be consistent across both test scripts. Consider either removing clean from unit_tests.sh as well or adding it back here.

Suggested change
compileTestJava
clean compileTestJava

Copilot uses AI. Check for mistakes.
set -x
echo "$integration_test_code"
echo "./gradlew \
-Dspring.profiles.active=${test_profile} \
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The variable test_profile is not quoted in this echo statement, which is inconsistent with lines 112, 121, and throughout the rest of the file where it is properly quoted. While this is just echoed output in a non-execution path, consistency in quoting practices helps prevent potential issues if the code is refactored or the variable contains special characters.

Suggested change
-Dspring.profiles.active=${test_profile} \
-Dspring.profiles.active=\"${test_profile}\" \

Copilot uses AI. Check for mistakes.
@duanemay duanemay merged commit 8334278 into develop Dec 16, 2025
33 checks passed
@duanemay duanemay deleted the testnames branch December 16, 2025 17:59
@github-project-automation github-project-automation bot moved this from Pending Merge | Prioritized to Done in Foundational Infrastructure Working Group Dec 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants