chore: Added capability of running ITs using ok-to-test #38355
Conversation
WalkthroughThis pull request introduces enhancements to GitHub Actions workflows and associated scripts, focusing on adding a new Changes
Sequence DiagramsequenceDiagram
participant PR as Pull Request
participant Parser as Tag Parser
participant Workflow as GitHub Actions
participant Tests as Integration Tests
PR ->> Parser: Trigger tag parsing
Parser -->> Workflow: Return parsed tags and 'its' flag
Workflow ->> Tests: Conditionally run tests based on 'its'
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitServerInitializerExtension.java (1)
40-40: Ensure null-safety on the injectedArtifactService.While Spring typically injects this service correctly, consider verifying that it is not null before use, especially for test scenarios.
.github/workflows/server-build.yml (1)
223-231: Remove trailing whitespace on line 223.The test execution logic is correct, but there's a formatting issue.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 223-223: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/pr-automation.yml(1 hunks).github/workflows/scripts/test-tag-parser.js(1 hunks).github/workflows/server-build.yml(3 hunks)app/server/appsmith-server/src/test/it/com/appsmith/server/git/templates/contexts/GitContext.java(1 hunks)app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitServerInitializerExtension.java(3 hunks)app/server/pom.xml(3 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pr-automation.yml
139-139: property "its" is not defined in object type {matrix: string; spec: string; tags: string}
(expression)
143-143: input "its" is typed as string by reusable workflow "./.github/workflows/server-build.yml". bool value cannot be assigned
(expression)
144-144: input "is-pg-build" is typed as string by reusable workflow "./.github/workflows/server-build.yml". bool value cannot be assigned
(expression)
🪛 yamllint (1.35.1)
.github/workflows/server-build.yml
[error] 223-223: trailing spaces
(trailing-spaces)
🔇 Additional comments (10)
app/server/appsmith-server/src/test/it/com/appsmith/server/git/templates/contexts/GitContext.java (1)
28-28: Storing the artifactType in the context store looks good.
This addition helps keep the artifact type readily accessible for downstream test extensions and setups.
app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitServerInitializerExtension.java (3)
4-4: Importing ArtifactService is appropriate.
Replacing ApplicationService with ArtifactService aligns with the move toward a dedicated artifact management approach.
59-59: Make sure the artifact type is present in the store.
If artifactType is missing or null, the subsequent SSH key generation may fail. Consider adding a fallback or appropriate error handling.
67-67: Verify the error handling of the createOrUpdateSshKeyPair call.
This call returns a reactive type. Ensure any exceptions are properly handled, e.g., logging or cleanup if the call fails.
app/server/pom.xml (4)
114-130: Unit test configuration is well-structured.
The maven-surefire-plugin is properly configured for unit tests with appropriate skip conditions and dependencies.
131-148: Integration test configuration is properly set up.
The maven-failsafe-plugin configuration correctly separates integration tests and includes necessary system properties.
46-48: LGTM! Test skip properties are well-structured.
The properties follow Maven conventions and provide granular control over test execution.
Let's verify these properties are used consistently across the project:
✅ Verification successful
Properties are correctly configured and used in the Maven build
The verification shows that the skip properties are used consistently within the app/server/pom.xml:
skipTestsis correctly used as the master switchskipITsandskipUTsare properly referenced in their respective build configurations- No conflicting or inconsistent usage found in other pom files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of skip properties in pom files
rg -U "skip(ITs|UTs|Tests)" -g "pom.xml"
Length of output: 356
80-103: Test source directories are properly configured.
The build-helper-maven-plugin configuration correctly adds multiple test source directories.
Let's verify the test directory structure exists:
.github/workflows/server-build.yml (2)
8-28: Workflow inputs are well-defined and documented.
The input parameters are clearly described with appropriate defaults.
Also applies to: 42-46
238-238: LGTM! Maven command correctly uses 'verify' phase.
The change to 'mvn verify' is appropriate for running both unit and integration tests.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/pr-automation.yml (1)
138-145:⚠️ Potential issueFix type mismatches in perform-it job inputs
The reusable workflow expects string inputs, but boolean expressions are being passed directly.
Apply this diff to fix the type mismatches:
perform-it: needs: [ parse-tags ] if: success() && needs.parse-tags.outputs.its == 'true' uses: ./.github/workflows/server-build.yml secrets: inherit with: - its: 'true' - is-pg-build: ${{ github.event.pull_request.base.ref == 'pg' }} + its: 'true' + is-pg-build: ${{ github.event.pull_request.base.ref == 'pg' && 'true' || 'false' }}🧰 Tools
🪛 actionlint (1.7.4)
144-144: input "its" is typed as string by reusable workflow "./.github/workflows/server-build.yml". bool value cannot be assigned
(expression)
145-145: input "is-pg-build" is typed as string by reusable workflow "./.github/workflows/server-build.yml". bool value cannot be assigned
(expression)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pr-automation.yml(2 hunks).github/workflows/scripts/test-tag-parser.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/scripts/test-tag-parser.js
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pr-automation.yml
144-144: input "its" is typed as string by reusable workflow "./.github/workflows/server-build.yml". bool value cannot be assigned
(expression)
145-145: input "is-pg-build" is typed as string by reusable workflow "./.github/workflows/server-build.yml". bool value cannot be assigned
(expression)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/server-build.yml (3)
Line range hint
8-46: Consider using a reusable inputs definitionThe input parameters are duplicated between
workflow_callandworkflow_dispatchtriggers. Consider using a composite action or a reusable workflow to define these inputs once.# Example approach using a composite action: # Create .github/actions/server-inputs/action.yml inputs: pr: description: "PR number for the workflow" required: false type: number # ... other inputs ... # Then in this file: on: workflow_call: inputs: ${{ uses: ./.github/actions/server-inputs }} workflow_dispatch: inputs: ${{ uses: ./.github/actions/server-inputs }}
224-230: LGTM! Consider adding a comment blockThe test execution logic correctly handles the separation of unit and integration tests. Consider adding a comment block explaining the mutual exclusivity of these test types.
+ # Note: This workflow is designed to run either unit tests or integration tests, + # but not both simultaneously. The `its` parameter determines which type to run. # Check if this run is requesting for unit tests or integration tests # As of today, we don't expect both to get triggered in the same workflow
223-223: Remove trailing whitespaceThere's a trailing whitespace on line 223.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 223-223: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/server-build.yml(4 hunks)app/server/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/server/README.md
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/server-build.yml
[error] 223-223: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/server-build.yml (1)
384-384: Verify artifact upload skip behavior
The condition to skip artifact upload during integration tests looks correct, but let's verify this doesn't impact dependent workflows.
✅ Verification successful
Skipping artifact upload for integration tests is safe
The condition to skip artifact upload during integration tests is correct and safe. Several workflows depend on the server-build artifact, but they all use the reusable workflow with their own parameters and are not affected by the integration test runs:
- github-release.yml: Uses artifacts for release packaging
- build-docker-image.yml: Downloads artifacts for Docker image creation
- test-build-docker-image.yml: Uses artifacts for testing and Docker builds
- pr-cypress.yml: Uses artifacts for Cypress testing
Each workflow either calls server-build.yml with its own parameters or downloads previously created artifacts, so skipping artifact upload during integration tests won't impact them.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for workflows that depend on the server-build artifact
# Search for workflow files that download the server-build artifact
echo "Checking for dependent workflows..."
rg -l "uses: actions/download-artifact@.*\n.*name: server-build" .github/workflows/
Length of output: 358
Script:
#!/bin/bash
# Description: Check for workflows that depend on the server-build artifact with multiline support
# Search for workflow files that download the server-build artifact
echo "Checking for dependent workflows..."
rg -U "uses: actions/download-artifact@.*\n.*name: server-build" .github/workflows/
# Also check for direct references to server-build artifact
echo -e "\nChecking for direct references to server-build artifact..."
rg "server-build" .github/workflows/ -C 2
Length of output: 13295
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/server-integration-tests.yml (1)
83-109: Inconsistency in test execution logging.The workflow uses
mvn verifybut logs tomvn_integration_test.log. This could cause confusion with the log parsing logic.Apply this diff to maintain consistency:
- mvn verify -DskipUTs=true "${args[@]}" | tee mvn_integration_test.log + mvn verify -DskipUTs=true "${args[@]}" | tee mvn_verify.log🧰 Tools
🪛 actionlint (1.7.4)
93-93: property "run_result" is not defined in object type {}
(expression)
.github/workflows/server-build.yml (1)
214-220: Improve test execution clarity.The test execution logic could be more explicit about the mutual exclusivity of unit and integration tests.
Add a comment explaining the test execution strategy:
+ # Note: This workflow is designed to run either unit tests or integration tests exclusively, + # but not both in the same run. This is controlled by the 'its' input parameter. if [[ "${{ inputs.its }}" == "true" ]]; then args+=("-DskipUTs=true") else args+=("-DskipITs=true") fiapp/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesIT.java (1)
215-216: LGTM! Consider enhancing the RTS prerequisite checkThe comment about RTS requirement is valuable. However, we could make this more robust by adding a programmatic check.
Consider adding a prerequisite check before the test:
@TestTemplate @WithUserDetails(value = "api_user") void test(GitContext gitContext, ExtensionContext extensionContext) throws IOException, GitAPIException, InterruptedException { + // Verify RTS is running + try { + // Add RTS health check here + new Socket("localhost", RTS_PORT).close(); + } catch (IOException e) { + throw new IllegalStateException("RTS must be running locally before executing this test", e); + } ExtensionContext.Store contextStore = extensionContext.getStore(ExtensionContext.Namespace.create(ArtifactBuilderExtension.class));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/pr-automation.yml(2 hunks).github/workflows/server-build.yml(3 hunks).github/workflows/server-integration-tests.yml(1 hunks)app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesIT.java(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/server-integration-tests.yml
43-43: property "run_result" is not defined in object type {}
(expression)
43-43: property "changed-files-specific" is not defined in object type {}
(expression)
50-50: property "skip-tests" is not defined in object type {is-pg-build: string; pr: number}
(expression)
93-93: property "run_result" is not defined in object type {}
(expression)
.github/workflows/server-build.yml
374-374: property "its" is not defined in object type {branch: string; is-pg-build: string; pr: number; skip-tests: string}
(expression)
.github/workflows/pr-automation.yml
145-145: input "is-pg-build" is typed as string by reusable workflow "./.github/workflows/server-integration-tests.yml". bool value cannot be assigned
(expression)
🪛 yamllint (1.35.1)
.github/workflows/server-integration-tests.yml
[error] 66-66: trailing spaces
(trailing-spaces)
[warning] 110-110: too many blank lines
(2 > 0) (empty-lines)
.github/workflows/server-build.yml
[error] 213-213: trailing spaces
(trailing-spaces)
🔇 Additional comments (5)
.github/workflows/server-integration-tests.yml (3)
3-16: LGTM! Well-structured workflow inputs.
The input parameters are well-defined with clear descriptions and appropriate types.
18-40: LGTM! Proper job setup and conditional logic.
The job configuration properly handles both PR and non-PR scenarios with appropriate checkout conditions.
55-82: LGTM! Comprehensive artifact handling.
The workflow properly manages server and RTS artifacts with appropriate extraction and setup steps.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 66-66: trailing spaces
(trailing-spaces)
.github/workflows/pr-automation.yml (1)
74-74: LGTM! Added missing its output.
The its output is properly added to the parse-tags job outputs, addressing the previous review comment.
.github/workflows/server-build.yml (1)
374-374: LGTM! Smart artifact upload condition.
The condition correctly skips artifact upload for integration test runs, which don't need to be deployed.
🧰 Tools
🪛 actionlint (1.7.4)
374-374: property "its" is not defined in object type {branch: string; is-pg-build: string; pr: number; skip-tests: string}
(expression)
.github/workflows/pr-automation.yml
Outdated
| perform-it: | ||
| needs: [ parse-tags, perform-test ] | ||
| if: success() && needs.parse-tags.outputs.its == 'true' | ||
| uses: ./.github/workflows/server-integration-tests.yml | ||
| secrets: inherit | ||
| with: | ||
| pr: ${{ github.event.number }} | ||
| is-pg-build: ${{ github.event.pull_request.base.ref == 'pg' }} |
There was a problem hiding this comment.
Fix type mismatch in is-pg-build parameter.
The is-pg-build parameter needs to be converted to string as the workflow expects a string input.
Apply this diff to fix the type mismatch:
- is-pg-build: ${{ github.event.pull_request.base.ref == 'pg' }}
+ is-pg-build: ${{ github.event.pull_request.base.ref == 'pg' && 'true' || 'false' }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| perform-it: | |
| needs: [ parse-tags, perform-test ] | |
| if: success() && needs.parse-tags.outputs.its == 'true' | |
| uses: ./.github/workflows/server-integration-tests.yml | |
| secrets: inherit | |
| with: | |
| pr: ${{ github.event.number }} | |
| is-pg-build: ${{ github.event.pull_request.base.ref == 'pg' }} | |
| perform-it: | |
| needs: [ parse-tags, perform-test ] | |
| if: success() && needs.parse-tags.outputs.its == 'true' | |
| uses: ./.github/workflows/server-integration-tests.yml | |
| secrets: inherit | |
| with: | |
| pr: ${{ github.event.number }} | |
| is-pg-build: ${{ github.event.pull_request.base.ref == 'pg' && 'true' || 'false' }} |
🧰 Tools
🪛 actionlint (1.7.4)
145-145: input "is-pg-build" is typed as string by reusable workflow "./.github/workflows/server-integration-tests.yml". bool value cannot be assigned
(expression)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/pr-cypress.yml (1)
6-9: LGTM! Consider adding parameter description.The input parameter is correctly defined. Consider adding a description field to document its purpose.
its: required: false type: string default: "false" + description: "Set to 'true' to run integration tests"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pr-automation.yml(2 hunks).github/workflows/pr-cypress.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pr-automation.yml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pr-cypress.yml
59-59: input "is-pg-build" is typed as string by reusable workflow "./.github/workflows/server-integration-tests.yml". bool value cannot be assigned
(expression)
🔇 Additional comments (1)
.github/workflows/pr-cypress.yml (1)
52-59:
Fix type mismatch in workflow input parameter.
The is-pg-build parameter needs to be converted to string to match the expected type in the reusable workflow.
- is-pg-build: ${{ github.event.pull_request.base.ref == 'pg' }}
+ is-pg-build: ${{ github.event.pull_request.base.ref == 'pg' && 'true' || 'false' }}Let's verify the referenced workflow exists:
✅ Verification successful
Boolean expression in workflow input needs string conversion
The verification confirms that is-pg-build is defined as a string input in the reusable workflow, so the suggested fix is correct. The boolean expression needs to be converted to a string value.
.github/workflows/server-integration-tests.ymlexpectsis-pg-buildas type: string with default: "false"- Current boolean expression
${{ github.event.pull_request.base.ref == 'pg' }}needs to be converted to string
- is-pg-build: ${{ github.event.pull_request.base.ref == 'pg' }}
+ is-pg-build: ${{ github.event.pull_request.base.ref == 'pg' && 'true' || 'false' }}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the server-integration-tests.yml workflow exists and check its inputs
if [ -f ".github/workflows/server-integration-tests.yml" ]; then
echo "Workflow exists"
grep -A 10 "workflow_call:" .github/workflows/server-integration-tests.yml
else
echo "Workflow not found"
fi
Length of output: 489
🧰 Tools
🪛 actionlint (1.7.4)
59-59: input "is-pg-build" is typed as string by reusable workflow "./.github/workflows/server-integration-tests.yml". bool value cannot be assigned
(expression)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/server-integration-tests.yml (1)
8-16: LGTM! Consider enhancing input descriptions.The input parameters are well-defined. However, the
prinput could benefit from a clearer description specifying that it's used for checking out the PR's merge commit.pr: - description: "This is the PR number in case the workflow is being called in a pull request" + description: "PR number used to checkout the merge commit for testing"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pr-cypress.yml(3 hunks).github/workflows/server-integration-tests.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pr-cypress.yml
59-59: input "is-pg-build" is typed as string by reusable workflow "./.github/workflows/server-integration-tests.yml". bool value cannot be assigned
(expression)
.github/workflows/server-integration-tests.yml
43-43: property "run_result" is not defined in object type {}
(expression)
43-43: property "changed-files-specific" is not defined in object type {}
(expression)
50-50: property "skip-tests" is not defined in object type {is-pg-build: string; pr: number}
(expression)
87-87: property "run_result" is not defined in object type {}
(expression)
🪛 yamllint (1.35.1)
.github/workflows/server-integration-tests.yml
[error] 60-60: trailing spaces
(trailing-spaces)
[warning] 104-104: too many blank lines
(2 > 0) (empty-lines)
🔇 Additional comments (3)
.github/workflows/server-integration-tests.yml (2)
21-24: LGTM! Fork safety and checkout logic are well implemented.
The conditional checkout logic correctly handles both PR and non-PR scenarios while maintaining security with fork checks.
Also applies to: 30-39
77-92: LGTM! Environment and database configuration are well structured.
The environment variables and conditional database URL selection are properly configured.
🧰 Tools
🪛 actionlint (1.7.4)
87-87: property "run_result" is not defined in object type {}
(expression)
.github/workflows/pr-cypress.yml (1)
85-85: LGTM! Job dependency update is correct.
Adding server-it to the needs array ensures proper workflow sequencing while always() condition maintains result reporting.
| - name: Set up JDK 17 | ||
| if: steps.run_result.outputs.run_result != 'success' && (steps.changed-files-specific.outputs.any_changed == 'true' || github.event_name == 'push' || github.event_name == 'workflow_dispatch' || github.event_name == 'schedule') |
There was a problem hiding this comment.
Fix JDK setup condition.
The condition references undefined step outputs run_result and changed-files-specific, which could prevent JDK setup from running.
- if: steps.run_result.outputs.run_result != 'success' && (steps.changed-files-specific.outputs.any_changed == 'true' || github.event_name == 'push' || github.event_name == 'workflow_dispatch' || github.event_name == 'schedule')
+ if: github.event_name == 'push' || github.event_name == 'workflow_dispatch' || github.event_name == 'workflow_call'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Set up JDK 17 | |
| if: steps.run_result.outputs.run_result != 'success' && (steps.changed-files-specific.outputs.any_changed == 'true' || github.event_name == 'push' || github.event_name == 'workflow_dispatch' || github.event_name == 'schedule') | |
| - name: Set up JDK 17 | |
| if: github.event_name == 'push' || github.event_name == 'workflow_dispatch' || github.event_name == 'workflow_call' |
🧰 Tools
🪛 actionlint (1.7.4)
43-43: property "run_result" is not defined in object type {}
(expression)
43-43: property "changed-files-specific" is not defined in object type {}
(expression)
| if [[ "${{ steps.run_result.outputs.run_result }}" == "failedtest" ]]; then | ||
| failed_tests="${{ steps.failed_tests.outputs.tests }}" | ||
| args+=("-DfailIfNoTests=false" "-Dsurefire.failIfNoSpecifiedTests=false" "-Dtest=${failed_tests}") |
There was a problem hiding this comment.
Fix test retry logic.
The condition references undefined step outputs run_result and failed_tests, which could cause issues with test retries.
- if [[ "${{ steps.run_result.outputs.run_result }}" == "failedtest" ]]; then
- failed_tests="${{ steps.failed_tests.outputs.tests }}"
+ if [[ -n "${FAILED_TESTS:-}" ]]; then
+ failed_tests="${FAILED_TESTS}"Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/server-integration-tests.yml (2)
42-43:⚠️ Potential issueFix JDK setup condition.
The condition references undefined step outputs which could prevent JDK setup from running.
- if: steps.run_result.outputs.run_result != 'success' && (steps.changed-files-specific.outputs.any_changed == 'true' || github.event_name == 'push' || github.event_name == 'workflow_dispatch' || github.event_name == 'schedule') + if: github.event_name == 'push' || github.event_name == 'workflow_dispatch' || github.event_name == 'workflow_call'🧰 Tools
🪛 actionlint (1.7.4)
43-43: property "run_result" is not defined in object type {}
(expression)
43-43: property "changed-files-specific" is not defined in object type {}
(expression)
96-98:⚠️ Potential issueFix test retry logic.
The condition references undefined step outputs which could cause issues with test retries.
- if [[ "${{ steps.run_result.outputs.run_result }}" == "failedtest" ]]; then - failed_tests="${{ steps.failed_tests.outputs.tests }}" + if [[ -n "${FAILED_TESTS:-}" ]]; then + failed_tests="${FAILED_TESTS}"
🧹 Nitpick comments (2)
.github/workflows/server-integration-tests.yml (2)
38-39: Add comment explaining the PR=0 condition.Consider adding a comment explaining that PR=0 represents non-pull request scenarios for better maintainability.
55-72: Remove trailing whitespace.Remove trailing whitespace on line 60 to maintain consistent formatting.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 60-60: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/server-integration-tests.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/server-integration-tests.yml
43-43: property "run_result" is not defined in object type {}
(expression)
43-43: property "changed-files-specific" is not defined in object type {}
(expression)
50-50: property "skip-tests" is not defined in object type {is-pg-build: string; pr: number}
(expression)
87-87: property "run_result" is not defined in object type {}
(expression)
🪛 yamllint (1.35.1)
.github/workflows/server-integration-tests.yml
[error] 60-60: trailing spaces
(trailing-spaces)
[warning] 104-104: too many blank lines
(2 > 0) (empty-lines)
🔇 Additional comments (2)
.github/workflows/server-integration-tests.yml (2)
1-17: LGTM! Well-structured workflow configuration.
The workflow triggers and input parameters are properly defined with clear descriptions.
18-27: LGTM! Secure job configuration.
Good security practice checking repository ownership before running tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/server-integration-tests.yml (2)
51-52: Consider enhancing PostgreSQL container configuration.While the current setup works, consider adding:
- Health check to ensure PostgreSQL is ready
- Volume mount for data persistence
- Environment variables for database name and user
docker run --name appsmith-pg -p 5432:5432 -d -e POSTGRES_PASSWORD=password postgres:alpine postgres -N 1500 +docker run --name appsmith-pg \ + -p 5432:5432 \ + -e POSTGRES_PASSWORD=password \ + -e POSTGRES_DB=appsmith \ + -e POSTGRES_USER=appsmith \ + -v pgdata:/var/lib/postgresql/data \ + --health-cmd pg_isready \ + --health-interval=10s \ + --health-timeout=5s \ + --health-retries=5 \ + -d postgres:alpine postgres -N 1500
59-59: Minor formatting issues.
- Remove trailing spaces on line 59
- Remove extra blank lines at the end of file
Also applies to: 98-98
🧰 Tools
🪛 yamllint (1.35.1)
[error] 59-59: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/server-integration-tests.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/server-integration-tests.yml
[error] 59-59: trailing spaces
(trailing-spaces)
[warning] 98-98: too many blank lines
(2 > 0) (empty-lines)
🔇 Additional comments (3)
.github/workflows/server-integration-tests.yml (3)
1-17: LGTM! Well-structured workflow configuration.
The workflow triggers and input parameters are properly defined with appropriate descriptions and types.
18-27: LGTM! Good security practice with repository validation.
The job configuration includes proper security checks to prevent unauthorized executions from forked repositories.
28-40: LGTM! Proper handling of PR and non-PR checkouts.
The checkout configuration correctly handles both scenarios using mutually exclusive conditions.
| - name: Run rts using the untarred files | ||
| run: | | ||
| nohup -- node app/client/packages/rts/dist/bundle/server.js & | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add verification for RTS server startup.
The RTS server is started in the background, but there's no verification that it started successfully.
nohup -- node app/client/packages/rts/dist/bundle/server.js &
+# Wait for RTS server to be ready
+for i in {1..30}; do
+ if curl -s http://localhost:8091/health >/dev/null; then
+ echo "RTS server is ready"
+ break
+ fi
+ if [ $i -eq 30 ]; then
+ echo "RTS server failed to start"
+ exit 1
+ fi
+ echo "Waiting for RTS server... ($i/30)"
+ sleep 1
+done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run rts using the untarred files | |
| run: | | |
| nohup -- node app/client/packages/rts/dist/bundle/server.js & | |
| - name: Run rts using the untarred files | |
| run: | | |
| nohup -- node app/client/packages/rts/dist/bundle/server.js & | |
| # Wait for RTS server to be ready | |
| for i in {1..30}; do | |
| if curl -s http://localhost:8091/health >/dev/null; then | |
| echo "RTS server is ready" | |
| break | |
| fi | |
| if [ $i -eq 30 ]; then | |
| echo "RTS server failed to start" | |
| exit 1 | |
| fi | |
| echo "Waiting for RTS server... ($i/30)" | |
| sleep 1 | |
| done |
| args=() | ||
|
|
||
| # Run tests and capture logs | ||
| mvn verify -DskipUTs=true "${args[@]}" | tee mvn_integration_test.log | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test failure handling and retry mechanism.
The current implementation doesn't handle test failures or provide a retry mechanism for flaky tests.
args=()
+max_retries=2
+retry_count=0
+
+while [ $retry_count -lt $max_retries ]; do
+ # Run tests and capture logs
+ if mvn verify -DskipUTs=true "${args[@]}" | tee mvn_integration_test.log; then
+ echo "Tests passed successfully"
+ exit 0
+ fi
+
+ retry_count=$((retry_count + 1))
+ if [ $retry_count -lt $max_retries ]; then
+ echo "Tests failed, attempting retry $retry_count of $max_retries"
+ sleep 30 # Wait before retry
+ fi
+done
-# Run tests and capture logs
-mvn verify -DskipUTs=true "${args[@]}" | tee mvn_integration_test.log
+echo "Tests failed after $max_retries attempts"
+exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| args=() | |
| # Run tests and capture logs | |
| mvn verify -DskipUTs=true "${args[@]}" | tee mvn_integration_test.log | |
| args=() | |
| max_retries=2 | |
| retry_count=0 | |
| while [ $retry_count -lt $max_retries ]; do | |
| # Run tests and capture logs | |
| if mvn verify -DskipUTs=true "${args[@]}" | tee mvn_integration_test.log; then | |
| echo "Tests passed successfully" | |
| exit 0 | |
| fi | |
| retry_count=$((retry_count + 1)) | |
| if [ $retry_count -lt $max_retries ]; then | |
| echo "Tests failed, attempting retry $retry_count of $max_retries" | |
| sleep 30 # Wait before retry | |
| fi | |
| done | |
| echo "Tests failed after $max_retries attempts" | |
| exit 1 |
Failed server tests
|
1 similar comment
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/server-build.yml (2)
8-23: Enhance input parameter descriptions for better clarity.While the descriptions are functional, they could be more informative:
- For
skip-tests: Specify what tests are skipped- For
is-pg-build: Clarify what PG means (PostgreSQL)- description: "Skip tests flag" + description: "Skip running unit tests if set to true" - description: "Flag for PG build" + description: "Use PostgreSQL instead of MongoDB if set to true"
213-213: LGTM! Clean implementation of array-based Maven arguments.The array-based approach for Maven test arguments is clean and maintainable. Consider extracting the test result processing logic into a separate script file for better maintainability.
.github/workflows/server-integration-tests.yml (2)
28-35: Add Redis container configuration and health check.The Redis service container lacks resource limits and health check configuration, which could affect test reliability.
redis: image: redis + options: >- + --health-cmd "redis-cli ping" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + env: + REDIS_MAXMEMORY: "256mb" + REDIS_MAXMEMORY_POLICY: "allkeys-lru" ports: - 6379:6379
57-62: Add PostgreSQL container health check.The PostgreSQL container startup should be verified before proceeding.
docker run --name appsmith-pg -p 5432:5432 -d -e POSTGRES_PASSWORD=password postgres:alpine postgres -N 1500 +# Wait for PostgreSQL to be ready +for i in {1..30}; do + if docker exec appsmith-pg pg_isready -U postgres >/dev/null 2>&1; then + echo "PostgreSQL is ready" + break + fi + if [ $i -eq 30 ]; then + echo "PostgreSQL failed to start" + exit 1 + fi + echo "Waiting for PostgreSQL... ($i/30)" + sleep 1 +done
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/server-build.yml(2 hunks).github/workflows/server-integration-tests.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/server-integration-tests.yml
[error] 68-68: trailing spaces
(trailing-spaces)
[warning] 108-108: too many blank lines
(2 > 0) (empty-lines)
🔇 Additional comments (4)
.github/workflows/server-integration-tests.yml (4)
1-17: LGTM! Well-structured workflow configuration.
The workflow triggers and inputs are properly defined with appropriate descriptions and default values.
37-49: LGTM! Proper checkout configuration.
The checkout steps correctly handle both PR and non-PR scenarios using the latest checkout action.
81-84: Add RTS server health check.
The RTS server is started in the background without verifying its successful startup.
85-107: Add test failure handling and retry mechanism.
The current implementation doesn't handle test failures or provide a retry mechanism for flaky tests.
Failed server tests
|
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Git" it=true
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12548211703
Commit: 28ec34d
Cypress dashboard.
Tags:
@tag.GitSpec:
Mon, 30 Dec 2024 15:48:17 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Chores
Documentation