[Don't merge] Added staging tests to CI/CD #687
Conversation
WalkthroughThe The The A new integration test suite was added in Possibly related PRs
Suggested labels
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cicd.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/cicd.yml
244-244: property "region" is not defined in object type {}
(expression)
248-248: property "region" is not defined in object type {}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build, Test and Push
- GitHub Check: Build, Test and Push
.github/workflows/cicd.yml
Outdated
| - name: "Deploy Tests to Cloud Run" | ||
| if: ${{ matrix.region == 'europe-west4' }} | ||
| uses: google-github-actions/deploy-cloudrun@v2 | ||
| with: | ||
| image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }} | ||
| region: ${{ matrix.region }} | ||
| job: stg-pr-${{ github.event.pull_request.number }}-tests | ||
| flags: --command="yarn _test:single" --wait --execute-now | ||
| skip_default_labels: true | ||
| labels: |- | ||
| commit-sha=${{ github.sha }} | ||
|
|
There was a problem hiding this comment.
Critical: matrix.region undefined in deploy-pr job, test step never runs & wrong image tag
In the deploy-pr job, there is no strategy.matrix.region, so:
if: ${{ matrix.region == 'europe-west4' }}
always evaluates to false and the "Deploy Tests to Cloud Run" step will never execute. Additionally, the image is incorrectly using the migrations_docker_version output instead of activitypub_docker_version, so even if it ran it would deploy the wrong container.
Apply this diff:
@@ -243,7 +243,7 @@
- name: "Deploy Tests to Cloud Run"
- if: ${{ matrix.region == 'europe-west4' }}
+ # Only run tests when ephemeral staging is enabled
+ if: ${{ steps.check-labels.outputs.is_ephemeral_staging == 'true' }}
uses: google-github-actions/deploy-cloudrun@v2
with:
- image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }}
+ image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.activitypub_docker_version }}
region: europe-west4
job: stg-pr-${{ github.event.pull_request.number }}-tests
flags: --command="yarn _test:single" --wait --execute-nowCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 actionlint (1.7.7)
244-244: property "region" is not defined in object type {}
(expression)
248-248: property "region" is not defined in object type {}
(expression)
🤖 Prompt for AI Agents
In .github/workflows/cicd.yml around lines 243 to 254, the "Deploy Tests to
Cloud Run" step uses an undefined matrix.region in its if condition, causing it
to never run, and it references the wrong image tag output. Fix this by removing
or correcting the if condition to ensure the step runs as intended, and update
the image tag to use the correct output variable activitypub_docker_version
instead of migrations_docker_version.
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
402f931 to
b1dfabf
Compare
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
b1dfabf to
3ce9e17
Compare
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
3ce9e17 to
70bba90
Compare
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
70bba90 to
d9c3583
Compare
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
d9c3583 to
33a933f
Compare
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
33a933f to
d20d4f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/test/db.ts (1)
69-79: 🛠️ Refactor suggestionUpdate cleanup connection to use socket path if available
The cleanup connection in the
afterAllhook doesn't use the socket path connection option that was added to the other connections. For consistency, consider updating it to use the same connection logic.afterAll(async () => { await dbClient.destroy(); const systemClient = knex({ client: 'mysql2', - connection: { - host: process.env.MYSQL_HOST, - port: Number.parseInt(process.env.MYSQL_PORT!), - user: process.env.MYSQL_USER, - password: process.env.MYSQL_PASSWORD, - database: 'mysql', - timezone: '+00:00', - }, + connection: process.env.MYSQL_SOCKET_PATH + ? { + socketPath: process.env.MYSQL_SOCKET_PATH, + user: process.env.MYSQL_USER, + password: process.env.MYSQL_PASSWORD, + database: 'mysql', + timezone: '+00:00', + } + : { + host: process.env.MYSQL_HOST, + port: Number.parseInt(process.env.MYSQL_PORT!), + user: process.env.MYSQL_USER, + password: process.env.MYSQL_PASSWORD, + database: 'mysql', + timezone: '+00:00', + }, }); await systemClient.raw(`DROP DATABASE ${dbName}`); await systemClient.destroy(); });If you implement the
getDbConnectionConfigfunction suggested earlier, you could also reuse it here with a parameter to override the database name:const systemClient = knex({ client: 'mysql2', connection: { ...getDbConnectionConfig(), database: 'mysql' }, });
🧹 Nitpick comments (1)
src/test/db.ts (1)
12-27: Enhance connection configuration by applying DRY principlesThe connection configuration is duplicated for both the
systemClientanddbClientobjects (lines 12-27 and 49-64). Consider extracting this logic into a reusable function to improve maintainability.You could create a function that returns the appropriate connection configuration:
function getDbConnectionConfig() { return process.env.MYSQL_SOCKET_PATH ? { socketPath: process.env.MYSQL_SOCKET_PATH, user: process.env.MYSQL_USER, password: process.env.MYSQL_PASSWORD, database: process.env.MYSQL_DATABASE, timezone: '+00:00', } : { host: process.env.MYSQL_HOST, port: Number.parseInt(process.env.MYSQL_PORT!), user: process.env.MYSQL_USER, password: process.env.MYSQL_PASSWORD, database: process.env.MYSQL_DATABASE, timezone: '+00:00', }; }Then use it in both places:
const systemClient = knex({ client: 'mysql2', connection: getDbConnectionConfig(), }); // And later const dbClient = knex({ client: 'mysql2', connection: getDbConnectionConfig(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/cicd.yml(3 hunks)Dockerfile(1 hunks)src/test/db.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Dockerfile
- .github/workflows/cicd.yml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: (ephemeral staging) Deploy
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
d20d4f6 to
2d10476
Compare
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
a065b74 to
c28c7cf
Compare
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
c28c7cf to
e4dff58
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/test/db.ts (1)
74-89: 🛠️ Refactor suggestionInconsistent connection method in afterAll cleanup.
The
afterAllhook doesn't support socket path connections like the rest of the code. It always uses TCP connections (host/port) for cleanup, which could cause issues in environments configured to use socket connections.For consistency, update the afterAll cleanup to use the same connection logic:
afterAll(async () => { await dbClient.destroy(); const systemClient = knex({ client: 'mysql2', - connection: { - host: process.env.MYSQL_HOST, - port: Number.parseInt(process.env.MYSQL_PORT!), - user: process.env.MYSQL_USER, - password: process.env.MYSQL_PASSWORD, - database: 'mysql', - timezone: '+00:00', - }, + connection: process.env.MYSQL_SOCKET_PATH + ? { + socketPath: process.env.MYSQL_SOCKET_PATH, + user: process.env.MYSQL_USER, + password: process.env.MYSQL_PASSWORD, + database: 'mysql', + timezone: '+00:00', + } + : { + host: process.env.MYSQL_HOST, + port: Number.parseInt(process.env.MYSQL_PORT!), + user: process.env.MYSQL_USER, + password: process.env.MYSQL_PASSWORD, + database: 'mysql', + timezone: '+00:00', + }, }); await systemClient.raw(`DROP DATABASE ${dbName}`); await systemClient.destroy(); });
🧹 Nitpick comments (2)
src/test/db.ts (2)
40-49: Improved table cloning strategy, but consider foreign key handling.The new approach of using
SHOW CREATE TABLEand filtering out constraints is more sophisticated than the previous method. However, it might still have issues with foreign keys or other complex relationships.Consider adding specific handling for foreign keys if your schema uses them:
const createTableSql = createTableResult[0]['Create Table'] .replace('CREATE TABLE ', `CREATE TABLE \`${dbName}\`.`) .split('\n') - .filter((line: string) => !line.trim().startsWith('CONSTRAINT')) + .filter((line: string) => { + // Filter out constraints but keep track of foreign keys for later if needed + const isConstraint = line.trim().startsWith('CONSTRAINT'); + return !isConstraint; + }) .join('\n') .replace(/,\n\)/, '\n)'); // clean up trailing comma
9-92: Consider adding error handling for database operations.The function lacks comprehensive error handling. If database operations fail, errors might not be properly caught, and resources might not be cleaned up correctly.
Consider wrapping the database operations in try-catch blocks and ensuring proper cleanup:
export async function createTestDb() { + let systemClient; + let dbClient; + let dbName; + + try { const systemClient = knex({ // ... existing configuration }); const dbName = `${process.env.MYSQL_DATABASE?.includes('pr-') ? ... }`; await systemClient.raw(`CREATE DATABASE ${dbName}`); // ... clone tables logic await systemClient.destroy(); const dbClient = knex({ // ... existing configuration }); // Setup cleanup in afterAll afterAll(async () => { + try { await dbClient?.destroy(); const cleanupClient = knex({ // ... configuration with socket path support }); await cleanupClient.raw(`DROP DATABASE ${dbName}`); await cleanupClient.destroy(); + } catch (error) { + console.error('Error during test database cleanup:', error); + } }); return dbClient; + } catch (error) { + // Clean up resources if initialization fails + await systemClient?.destroy(); + await dbClient?.destroy(); + + // Try to drop the database if it was created + if (dbName) { + try { + const cleanupClient = knex({ + // ... configuration with socket path support + }); + await cleanupClient.raw(`DROP DATABASE IF EXISTS ${dbName}`); + await cleanupClient.destroy(); + } catch (cleanupError) { + console.error('Error during cleanup after initialization failure:', cleanupError); + } + } + + throw error; // Re-throw the original error + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/cicd.yml(3 hunks)Dockerfile(1 hunks)src/test/db.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Dockerfile
- .github/workflows/cicd.yml
🔇 Additional comments (3)
src/test/db.ts (3)
12-27: Support for both socket and TCP connections is a good enhancement.The addition of conditional connection logic based on the presence of
MYSQL_SOCKET_PATHimproves flexibility and allows the tests to run in different environments, such as Cloud Run where socket connections might be preferred.
30-30: Smart database naming strategy for PR environments.The enhanced naming logic makes it easier to identify test databases in PR environments by including a sanitized version of the PR database name as a prefix when applicable. This is helpful for debugging and monitoring in CI/CD pipelines.
56-71: Correctly using the test database name in connection configuration.This change properly connects to the temporary test database using the
dbNamevariable instead ofprocess.env.MYSQL_DATABASE, which resolves the issue mentioned in the past review comment.
rmgpinto
changed the title
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
e4dff58 to
a4cf6e8
Compare
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
a4cf6e8 to
28cacc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/cicd.yml (2)
243-254:⚠️ Potential issueCritical: Test jobs running the wrong container image
The
Deploy Tests to Cloud Runstep in thedeploy-prjob still references the migrations image tag (migrations_docker_version) instead of the ActivityPub service image. This will run tests against the wrong container.
Apply this change:- image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }} + image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.activitypub_docker_version }}
329-340:⚠️ Potential issueCritical: Test jobs running the wrong container image
The
Deploy Tests to Cloud Runstep in thedeploy-stagingjob still uses the migrations image tag (migrations_docker_version). It should use the ActivityPub service image:- image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }} + image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.activitypub_docker_version }}
🧹 Nitpick comments (1)
.github/workflows/cicd.yml (1)
105-107: Remove obsolete commented test stepThe inline
Run Testsstep has been commented out. If it’s no longer needed, consider removing these commented lines to keep the workflow clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/cicd.yml(3 hunks)Dockerfile(1 hunks)src/test/db.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Dockerfile
- src/test/db.ts
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
28cacc3 to
27e77fd
Compare
closes https://linear.app/ghost/issue/PROD-1779 - Added a CI/CD step that runs tests in staging. This will attempt to catch errors in GCP components before being shipped to production.
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
27e77fd to
b5e5231
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/storage/gcloud-storage/gcp-storage.service.integration.test.ts (3)
1-16: Well-structured imports and setup for the integration test.The imports, logger mock, and test constants are well organized. However, I notice the test might be assuming the
GCP_BUCKET_NAMEenvironment variable is set (used later on line 141).Consider adding validation for required environment variables at the beginning of the test to provide clearer error messages if they're missing.
25-54: Good validation logic for successful file upload.The test appropriately checks both the emulator and production scenarios, with different verification strategies for each environment.
Consider adding a teardown step to clean up the files uploaded during tests to prevent accumulation of test data in your storage bucket.
92-113: Comprehensive URL verification test.The test correctly saves a file first and then verifies its URL. The validation logic is thorough.
To make the test more robust against potential race conditions when running against real GCP services, consider adding a small delay between saving the file and verifying it, as cloud storage systems can sometimes have propagation delays.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/storage/gcloud-storage/assets/dog.jpgis excluded by!**/*.jpg
📒 Files selected for processing (1)
src/storage/gcloud-storage/gcp-storage.service.integration.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, Test and Push
🔇 Additional comments (6)
src/storage/gcloud-storage/gcp-storage.service.integration.test.ts (6)
17-23: LGTM - Good test initialization practice.The service is properly initialized in a
beforeAllblock, which is good practice to avoid repetitive setup code.
56-72: LGTM - Proper size limit testing.Good test for the file size limit, creating a buffer larger than the allowed size and verifying the correct error is returned.
74-90: LGTM - Good file type validation test.The test properly verifies that unsupported file types are rejected with the correct error code.
115-123: LGTM - Good invalid URL test.The test properly checks that invalid URLs are rejected with the correct error code.
125-137: Good environment-aware testing for invalid paths.The test correctly handles different error codes based on whether the GCP emulator is being used.
139-151:✅ Verification successful
Good environment-aware testing for non-existent files.
The test appropriately uses environment variables to construct the test URL and correctly handles different error codes based on the testing environment.
Let's verify that the environment variable referenced is consistently used throughout the codebase:
🏁 Script executed:
#!/bin/bash # Check usage of GCP_BUCKET_NAME environment variable rg -A 3 "GCP_BUCKET_NAME" --type tsLength of output: 2131
🏁 Script executed:
#!/bin/bash # Display the implementation of verifyImageUrl to confirm error codes rg -A 20 "verifyImageUrl" src/storage/gcloud-storage/gcp-storage.service.tsLength of output: 950
🏁 Script executed:
#!/bin/bash # Search for file-not-found constant usage in implementation rg -C 5 "file-not-found" src/storage/gcloud-storage/gcp-storage.service.tsLength of output: 751
Approve environment-aware error handling test
The non-existent file test correctly validates both emulator and production paths:
- With
GCP_STORAGE_EMULATOR_HOSTset,verifyImageUrlreturnsinvalid-url.- Without the emulator, it returns
file-not-found.No further changes needed.
closes https://linear.app/ghost/issue/PROD-1779