Skip to content
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/pr-automation.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ jobs:
shell: bash
outputs:
tags: ${{ steps.parseTags.outputs.tags }}
its: ${{ steps.parseTags.outputs.its }}
Comment thread
nidhi-nair marked this conversation as resolved.
spec: ${{ steps.parseTags.outputs.spec }}
matrix: ${{ steps.checkAll.outputs.matrix }}
steps:
Expand Down Expand Up @@ -129,6 +130,7 @@ jobs:
uses: ./.github/workflows/pr-cypress.yml
secrets: inherit
with:
its: ${{ needs.parse-tags.outputs.its}}
tags: ${{ needs.parse-tags.outputs.tags}}
spec: ${{ needs.parse-tags.outputs.spec}}
matrix: ${{ needs.parse-tags.outputs.matrix}}
Expand Down
15 changes: 14 additions & 1 deletion .github/workflows/pr-cypress.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ name: Cypress test suite
on:
workflow_call:
inputs:
its:
required: false
type: string
default: "false"
tags:
required: true
type: string
Expand Down Expand Up @@ -45,6 +49,15 @@ jobs:
with:
pr: ${{ github.event.number }}

server-it:
needs: [ server-build, rts-build ]
if: success() && inputs.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' }}
Comment thread
nidhi-nair marked this conversation as resolved.

build-docker-image:
needs: [client-build, server-build, rts-build]
# Only run if the build step is successful
Expand All @@ -69,7 +82,7 @@ jobs:
matrix: ${{ inputs.matrix }}

ci-test-result:
needs: [ci-test]
needs: [ci-test, server-it]
# Only run if the ci-test with matrices step is successful
if: always()
runs-on: ubuntu-latest
Expand Down
32 changes: 22 additions & 10 deletions .github/workflows/scripts/test-tag-parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,25 +21,37 @@ module.exports = function ({core, context, github}) {
}

core.setOutput("tags", parseResult.tags ?? "");
core.setOutput("its", parseResult.its ?? "");
core.setOutput("spec", parseResult.spec ?? "");
}

function parseTags(body) {
const allTags = require(process.env.GITHUB_WORKSPACE + "/app/client/cypress/tags.js").Tag;

// "/ok-to-test" matcher. Takes precedence over the "/test" matcher.
const strictMatch = body.match(/^\/ok-to-test tags="(.+?)"/m)?.[1];
if (strictMatch) {
if (strictMatch === "@tag.All") {
return { tags: strictMatch };
}
const parts = strictMatch.split(/\s*,\s*/);
for (const part of parts) {
if (!allTags.includes(part)) {
throw new Error("Unknown tag: " + part);
const okToTestPattern = body.match(/^(\/ok-to-test) tags="(.+?)"( it=true)?/m);

if (okToTestPattern?.[1]) {
var response = {};
const tagsMatch = okToTestPattern?.[2];
if (tagsMatch) {
if (tagsMatch === "@tag.All") {
response = { tags: tagsMatch };
} else {
const parts = tagsMatch.split(/\s*,\s*/);
for (const part of parts) {
if (!allTags.includes(part)) {
throw new Error("Unknown tag: " + part);
}
}
response = { tags: tagsMatch };
}
}
return { tags: strictMatch };
const itsMatch = okToTestPattern?.[3];
if (itsMatch) {
response = { ...response, its: 'true' };
}
Comment thread
nidhi-nair marked this conversation as resolved.
return response;
}

// "/test" code-fence matcher.
Expand Down
18 changes: 14 additions & 4 deletions .github/workflows/server-build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,20 @@ on:
workflow_call:
inputs:
pr:
description: "This is the PR number in case the workflow is being called in a pull request"
description: "PR number for the workflow"
required: false
type: number
skip-tests:
description: "This is a boolean value in case the workflow is being called in build deploy-preview"
description: "Skip tests flag"
required: false
type: string
default: "false"
branch:
description: "This is the branch to be used for the build."
description: "Branch for the build"
required: false
type: string
is-pg-build:
description: "This is a boolean value in case the workflow is being called for a PG build"
description: "Flag for PG build"
required: false
type: string
default: "false"
Expand Down Expand Up @@ -210,6 +210,15 @@ jobs:
fi

args=()

# 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
if [[ "${{ inputs.its }}" == "true" ]]; then
args+=("-DskipUTs=true")
else
args+=("-DskipITs=true")
fi

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}")
Expand Down Expand Up @@ -362,6 +371,7 @@ jobs:

# Upload the build artifact so that it can be used by the test & deploy job in the workflow
- name: Upload server build bundle
if: inputs.its != 'true'
uses: actions/upload-artifact@v4
with:
name: server-build
Expand Down
104 changes: 104 additions & 0 deletions .github/workflows/server-integration-tests.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
name: Server Integrations Tests Workflow

on:
# This line enables manual triggering of this workflow.
workflow_dispatch:
workflow_call:
inputs:
pr:
description: "This is the PR number in case the workflow is being called in a pull request"
required: false
type: number
is-pg-build:
description: "Flag for PG build"
required: false
type: string
default: "false"

jobs:
run-tests:
runs-on: ubuntu-latest
if: |
github.event.pull_request.head.repo.full_name == github.repository ||
github.event_name == 'workflow_dispatch'
defaults:
run:
shell: bash

steps:
# Check out merge commit
- name: Fork based /ok-to-test checkout
if: inputs.pr != 0
uses: actions/checkout@v4
with:
ref: "refs/pull/${{ inputs.pr }}/merge"

# Checkout the code in the current branch in case the workflow is called because of a branch push event
- name: Checkout the head commit of the branch
if: inputs.pr == 0
uses: actions/checkout@v4

# Setup Java
- 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')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
- 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)

uses: actions/setup-java@v4
with:
distribution: "temurin"
java-version: "17"

- name: Conditionally start PostgreSQL
if: |
inputs.is-pg-build == 'true' && inputs.skip-tests != 'true'
run: |
Comment thread
nidhi-nair marked this conversation as resolved.
docker run --name appsmith-pg -p 5432:5432 -d -e POSTGRES_PASSWORD=password postgres:alpine postgres -N 1500

- name: Download the server build artifact
uses: actions/download-artifact@v4
with:
name: server-build
path: app/server/dist/

- name: Download the rts build artifact
uses: actions/download-artifact@v4
with:
name: rts-dist
path: app/client/packages/rts/dist

- name: Un-tar the rts folder
run: |
tar -xvf app/client/packages/rts/dist/rts-dist.tar -C app/client/packages/rts/
echo "Cleaning up the rts tar files"
rm app/client/packages/rts/dist/rts-dist.tar

- name: Run rts using the untarred files
run: |
node app/client/packages/rts/dist/bundle/server.js

Comment on lines +81 to +84

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
- 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

- name: Run only integration tests on server
env:
ACTIVE_PROFILE: test
APPSMITH_CLOUD_SERVICES_BASE_URL: "https://release-cs.appsmith.com"
APPSMITH_CLOUD_SERVICES_TEMPLATE_UPLOAD_AUTH: ${{ secrets.APPSMITH_CLOUD_SERVICES_TEMPLATE_UPLOAD_AUTH }}
APPSMITH_REDIS_URL: "redis://127.0.0.1:6379"
APPSMITH_ENCRYPTION_PASSWORD: "password"
APPSMITH_ENCRYPTION_SALT: "salt"
APPSMITH_ENVFILE_PATH: /tmp/dummy.env
APPSMITH_VERBOSE_LOGGING_ENABLED: false
run: |
if [[ "${{ inputs.is-pg-build }}" == "true" ]]; then
export APPSMITH_DB_URL="postgresql://postgres:password@localhost:5432/postgres"
else
export APPSMITH_DB_URL="mongodb://localhost:27017/mobtools"
fi

args=()

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}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

fi

# Run tests and capture logs
mvn verify -DskipUTs=true "${args[@]}" | tee mvn_integration_test.log
Comment thread
sagar-qa007 marked this conversation as resolved.

Comment on lines +93 to +107

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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


2 changes: 1 addition & 1 deletion app/server/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@

This is the server-side repository for the Appsmith framework.
<br><br>
For details on setting up your development machine, please refer to this [Setup Guide](../../contributions/ServerSetup.md).
For details on setting up your development machine, please refer to this [setup Guide](../../contributions/ServerSetup.md).
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,8 @@ void test(GitContext gitContext, ExtensionContext extensionContext) throws IOExc

assertThat(autoCommitResponseDTO).isNotNull();
AutoCommitResponseDTO.AutoCommitResponse autoCommitProgress = autoCommitResponseDTO.getAutoCommitResponse();
// This check requires RTS to be running on your local since client side changes come in from there
// Please make sure to run RTS before triggering this test
assertThat(autoCommitProgress).isEqualTo(AutoCommitResponseDTO.AutoCommitResponse.PUBLISHED);

// Wait for auto-commit to complete
Expand Down