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

chore: Added capability of running ITs using ok-to-test #38355

Merged
merged 19 commits into from
Jan 1, 2025
Merged
Show file tree
Hide file tree
Changes from all 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 }}
nidhi-nair marked this conversation as resolved.
Show resolved Hide 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' }}
nidhi-nair marked this conversation as resolved.
Show resolved Hide 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' };
}
return response;
}

// "/test" code-fence matcher.
Expand Down
9 changes: 5 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,7 @@ jobs:
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}")
Expand Down
108 changes: 108 additions & 0 deletions .github/workflows/server-integration-tests.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
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-8-cores
sagar-qa007 marked this conversation as resolved.
Show resolved Hide resolved
if: |
github.event.pull_request.head.repo.full_name == github.repository ||
github.event_name == 'workflow_dispatch'
defaults:
run:
shell: bash
# Service containers to run with this job. Required for running tests
services:
# Label used to access the service container
redis:
# Docker Hub image for Redis
image: redis
ports:
# Opens tcp port 6379 on the host and service container
- 6379:6379

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
uses: actions/setup-java@v4
with:
distribution: "temurin"
java-version: "17"

- name: Conditionally start PostgreSQL
if: |
inputs.is-pg-build == 'true'
run: |
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: |
nohup -- node app/client/packages/rts/dist/bundle/server.js &

Comment on lines +81 to +84
Copy link
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=()

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


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
Loading