Run full test suite in GitHub Actions with disposable test env#7
Conversation
![codra-app-personal[bot]](https://avatars.githubusercontent.com/in/3347243?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42186fb1a6
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| # Codra Environment Configuration Example | ||
| # Copy this file to .dev.vars for local development: cp .dev.vars.example .dev.vars | ||
| # ────────────────────────────────────────────────────────────────────────────── | ||
| # Codra local development environment example |
There was a problem hiding this comment.
Several environment variables (DASHBOARD_ALLOWED_USERS, GITHUB_APP_SLUG, CF_DLQ_ID, and BOT_USERNAME) have been removed from the .dev.vars.example file. If these variables are still required for the application to function correctly or for specific features (like DLQ management) to work, new developers will lack the necessary documentation to configure their local environment, leading to potential runtime errors.
| # Comma-separated list of GitHub usernames allowed to access the dashboard | ||
| DASHBOARD_ALLOWED_USERS="username1,username2" | ||
| # --- Integration tests --- | ||
| TEST_DATABASE_URL="postgresql://user:password@localhost:5432/codra" |
There was a problem hiding this comment.
The TEST_DATABASE_URL has been changed from postgresql://user:password@localhost:5432/codra_test to postgresql://user:password@localhost:5432/codra. This is inconsistent with the naming convention used for DATABASE_URL (codra_dev). It is recommended to keep the _test suffix to clearly distinguish the test database from other environments and prevent accidental data collisions.
| TEST_DATABASE_URL="postgresql://user:password@localhost:5432/codra" | |
| TEST_DATABASE_URL="postgresql://user:password@localhost:5432/codra_test" |
| env: | ||
| DATABASE_URL: postgresql://postgres:[email protected]:5432/codra_test | ||
| TEST_DATABASE_URL: postgresql://postgres:[email protected]:5432/codra_test | ||
| GITHUB_APP_SLUG: codra-test-app |
There was a problem hiding this comment.
The workflow hardcodes dummy values for sensitive parameters such as GITHUB_APP_WEBHOOK_SECRET, GITHUB_CLIENT_ID, and GITHUB_CLIENT_SECRET. While these are currently placeholders, hardcoding them in the repository is a security anti-pattern that can lead to accidental exposure of real secrets if developers follow this pattern. It is recommended to use GitHub Actions Secrets (e.g., ${{ secrets.GITHUB_CLIENT_SECRET }}) even for test environments.
| GITHUB_APP_SLUG: codra-test-app | |
| GITHUB_APP_WEBHOOK_SECRET: ${{ secrets.GITHUB_APP_WEBHOOK_SECRET }} | |
| GITHUB_CLIENT_ID: ${{ secrets.GITHUB_CLIENT_ID }} | |
| GITHUB_CLIENT_SECRET: ${{ secrets.GITHUB_CLIENT_SECRET }} |
| GITHUB_CLIENT_ID: fake-dashboard-client-id | ||
| GITHUB_CLIENT_SECRET: fake-dashboard-client-secret | ||
| AUTH_CALLBACK_URL: https://codra.test/auth/github/callback | ||
| APP_URL: https://codra.test |
There was a problem hiding this comment.
The DASHBOARD_ALLOWED_USERS environment variable contains a specific username (devarshishimpi). For better maintainability and to avoid using real user identities in CI/CD configurations, consider using a generic test user identifier.
| APP_URL: https://codra.test | |
| DASHBOARD_ALLOWED_USERS: test-user |
| APP_URL: requiredEnv('APP_URL'), | ||
| DASHBOARD_ALLOWED_USERS: requiredEnv('DASHBOARD_ALLOWED_USERS'), | ||
| get GEMINI_API_KEY() { return unusedEnv('GEMINI_API_KEY'); }, | ||
| BOT_USERNAME: requiredEnv('BOT_USERNAME'), |
There was a problem hiding this comment.
The 'ENVIRONMENT' property in 'createTestEnv' has been changed from a hardcoded 'test' value to a getter that calls 'unusedEnv()', which throws an Error. If any application code or test helper relies on the default 'ENVIRONMENT' being 'test' without providing it in the 'overrides' object, the tests will crash at runtime.
| BOT_USERNAME: requiredEnv('BOT_USERNAME'), | |
| ENVIRONMENT: 'test', |
| ENVIRONMENT: 'test', | ||
| CF_API_TOKEN: 'cf-api-token', | ||
| CF_ACCOUNT_ID: 'cf-account-id', | ||
| CF_DLQ_ID: 'cf-dlq-id', |
There was a problem hiding this comment.
Properties like 'APP_PRIVATE_KEY' were previously provided with dummy values. They are now getters that throw via 'unusedEnv()'. If the system under test performs validation on these keys during bootstrap (e.g., checking for a valid PEM format), the tests will fail immediately upon initialization unless these keys are explicitly provided in the environment or overrides.
| return value && value !== 'undefined' && value !== 'null' ? value : null; | ||
| } | ||
|
|
||
| function loadEnvFiles() { |
There was a problem hiding this comment.
The loadEnvFiles function implements a custom parser for .env files by splitting content by newlines and slicing at the first '='. This approach is fragile and fails to handle several common .env standards: 1) Inline comments (e.g., 'KEY=VALUE # comment' will result in the value being 'VALUE # comment'), 2) Multi-line quoted values (the line-by-line split breaks these), and 3) Quote stripping fails if there is any trailing whitespace or comments after the closing quote. Using a battle-tested library like 'dotenv' is recommended, or the parser should be enhanced to handle these edge cases.
| function loadEnvFiles() { | |
| // Consider using a library like dotenv or improving the regex: | |
| // Example for basic inline comment removal: | |
| // const lineWithoutComment = line.split('#')[0].trim(); |
| } | ||
|
|
||
| function usableEnvValue(value) { | ||
| return value && value !== 'undefined' && value !== 'null' ? value : null; |
There was a problem hiding this comment.
The usableEnvValue function uses a truthiness check (value && ...), which means an empty string ("") will be returned as null. While likely acceptable for database URLs, this pattern can cause bugs if other environment variables are loaded where an empty string is a distinct and valid configuration value different from null or undefined.
| return value && value !== 'undefined' && value !== 'null' ? value : null; | |
| return (value !== undefined && value !== null && value !== 'undefined' && value !== 'null') ? value : null; |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42186fb1a6
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| # Codra Environment Configuration Example | ||
| # Copy this file to .dev.vars for local development: cp .dev.vars.example .dev.vars | ||
| # ────────────────────────────────────────────────────────────────────────────── | ||
| # Codra local development environment example |
There was a problem hiding this comment.
Several environment variables (DASHBOARD_ALLOWED_USERS, GITHUB_APP_SLUG, CF_DLQ_ID, and BOT_USERNAME) have been removed from the .dev.vars.example file. If these variables are still required for the application to function correctly or for specific features (like DLQ management) to work, new developers will lack the necessary documentation to configure their local environment, leading to potential runtime errors.
| # Comma-separated list of GitHub usernames allowed to access the dashboard | ||
| DASHBOARD_ALLOWED_USERS="username1,username2" | ||
| # --- Integration tests --- | ||
| TEST_DATABASE_URL="postgresql://user:password@localhost:5432/codra" |
There was a problem hiding this comment.
The TEST_DATABASE_URL has been changed from postgresql://user:password@localhost:5432/codra_test to postgresql://user:password@localhost:5432/codra. This is inconsistent with the naming convention used for DATABASE_URL (codra_dev). It is recommended to keep the _test suffix to clearly distinguish the test database from other environments and prevent accidental data collisions.
| TEST_DATABASE_URL="postgresql://user:password@localhost:5432/codra" | |
| TEST_DATABASE_URL="postgresql://user:password@localhost:5432/codra_test" |
| env: | ||
| DATABASE_URL: postgresql://postgres:[email protected]:5432/codra_test | ||
| TEST_DATABASE_URL: postgresql://postgres:[email protected]:5432/codra_test | ||
| GITHUB_APP_SLUG: codra-test-app |
There was a problem hiding this comment.
The workflow hardcodes dummy values for sensitive parameters such as GITHUB_APP_WEBHOOK_SECRET, GITHUB_CLIENT_ID, and GITHUB_CLIENT_SECRET. While these are currently placeholders, hardcoding them in the repository is a security anti-pattern that can lead to accidental exposure of real secrets if developers follow this pattern. It is recommended to use GitHub Actions Secrets (e.g., ${{ secrets.GITHUB_CLIENT_SECRET }}) even for test environments.
| GITHUB_APP_SLUG: codra-test-app | |
| GITHUB_APP_WEBHOOK_SECRET: ${{ secrets.GITHUB_APP_WEBHOOK_SECRET }} | |
| GITHUB_CLIENT_ID: ${{ secrets.GITHUB_CLIENT_ID }} | |
| GITHUB_CLIENT_SECRET: ${{ secrets.GITHUB_CLIENT_SECRET }} |
| GITHUB_CLIENT_ID: fake-dashboard-client-id | ||
| GITHUB_CLIENT_SECRET: fake-dashboard-client-secret | ||
| AUTH_CALLBACK_URL: https://codra.test/auth/github/callback | ||
| APP_URL: https://codra.test |
There was a problem hiding this comment.
The DASHBOARD_ALLOWED_USERS environment variable contains a specific username (devarshishimpi). For better maintainability and to avoid using real user identities in CI/CD configurations, consider using a generic test user identifier.
| APP_URL: https://codra.test | |
| DASHBOARD_ALLOWED_USERS: test-user |
| APP_URL: requiredEnv('APP_URL'), | ||
| DASHBOARD_ALLOWED_USERS: requiredEnv('DASHBOARD_ALLOWED_USERS'), | ||
| get GEMINI_API_KEY() { return unusedEnv('GEMINI_API_KEY'); }, | ||
| BOT_USERNAME: requiredEnv('BOT_USERNAME'), |
There was a problem hiding this comment.
The 'ENVIRONMENT' property in 'createTestEnv' has been changed from a hardcoded 'test' value to a getter that calls 'unusedEnv()', which throws an Error. If any application code or test helper relies on the default 'ENVIRONMENT' being 'test' without providing it in the 'overrides' object, the tests will crash at runtime.
| BOT_USERNAME: requiredEnv('BOT_USERNAME'), | |
| ENVIRONMENT: 'test', |
| ENVIRONMENT: 'test', | ||
| CF_API_TOKEN: 'cf-api-token', | ||
| CF_ACCOUNT_ID: 'cf-account-id', | ||
| CF_DLQ_ID: 'cf-dlq-id', |
There was a problem hiding this comment.
Properties like 'APP_PRIVATE_KEY' were previously provided with dummy values. They are now getters that throw via 'unusedEnv()'. If the system under test performs validation on these keys during bootstrap (e.g., checking for a valid PEM format), the tests will fail immediately upon initialization unless these keys are explicitly provided in the environment or overrides.
| return value && value !== 'undefined' && value !== 'null' ? value : null; | ||
| } | ||
|
|
||
| function loadEnvFiles() { |
There was a problem hiding this comment.
The loadEnvFiles function implements a custom parser for .env files by splitting content by newlines and slicing at the first '='. This approach is fragile and fails to handle several common .env standards: 1) Inline comments (e.g., 'KEY=VALUE # comment' will result in the value being 'VALUE # comment'), 2) Multi-line quoted values (the line-by-line split breaks these), and 3) Quote stripping fails if there is any trailing whitespace or comments after the closing quote. Using a battle-tested library like 'dotenv' is recommended, or the parser should be enhanced to handle these edge cases.
| function loadEnvFiles() { | |
| // Consider using a library like dotenv or improving the regex: | |
| // Example for basic inline comment removal: | |
| // const lineWithoutComment = line.split('#')[0].trim(); |
| } | ||
|
|
||
| function usableEnvValue(value) { | ||
| return value && value !== 'undefined' && value !== 'null' ? value : null; |
There was a problem hiding this comment.
The usableEnvValue function uses a truthiness check (value && ...), which means an empty string ("") will be returned as null. While likely acceptable for database URLs, this pattern can cause bugs if other environment variables are loaded where an empty string is a distinct and valid configuration value different from null or undefined.
| return value && value !== 'undefined' && value !== 'null' ? value : null; | |
| return (value !== undefined && value !== null && value !== 'undefined' && value !== 'null') ? value : null; |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42186fb1a6
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| # Codra Environment Configuration Example | ||
| # Copy this file to .dev.vars for local development: cp .dev.vars.example .dev.vars | ||
| # ────────────────────────────────────────────────────────────────────────────── | ||
| # Codra local development environment example |
There was a problem hiding this comment.
Several environment variables (DASHBOARD_ALLOWED_USERS, GITHUB_APP_SLUG, CF_DLQ_ID, and BOT_USERNAME) have been removed from the .dev.vars.example file. If these variables are still required for the application to function correctly or for specific features (like DLQ management) to work, new developers will lack the necessary documentation to configure their local environment, leading to potential runtime errors.
| # Comma-separated list of GitHub usernames allowed to access the dashboard | ||
| DASHBOARD_ALLOWED_USERS="username1,username2" | ||
| # --- Integration tests --- | ||
| TEST_DATABASE_URL="postgresql://user:password@localhost:5432/codra" |
There was a problem hiding this comment.
The TEST_DATABASE_URL has been changed from postgresql://user:password@localhost:5432/codra_test to postgresql://user:password@localhost:5432/codra. This is inconsistent with the naming convention used for DATABASE_URL (codra_dev). It is recommended to keep the _test suffix to clearly distinguish the test database from other environments and prevent accidental data collisions.
| TEST_DATABASE_URL="postgresql://user:password@localhost:5432/codra" | |
| TEST_DATABASE_URL="postgresql://user:password@localhost:5432/codra_test" |
| env: | ||
| DATABASE_URL: postgresql://postgres:[email protected]:5432/codra_test | ||
| TEST_DATABASE_URL: postgresql://postgres:[email protected]:5432/codra_test | ||
| GITHUB_APP_SLUG: codra-test-app |
There was a problem hiding this comment.
The workflow hardcodes dummy values for sensitive parameters such as GITHUB_APP_WEBHOOK_SECRET, GITHUB_CLIENT_ID, and GITHUB_CLIENT_SECRET. While these are currently placeholders, hardcoding them in the repository is a security anti-pattern that can lead to accidental exposure of real secrets if developers follow this pattern. It is recommended to use GitHub Actions Secrets (e.g., ${{ secrets.GITHUB_CLIENT_SECRET }}) even for test environments.
| GITHUB_APP_SLUG: codra-test-app | |
| GITHUB_APP_WEBHOOK_SECRET: ${{ secrets.GITHUB_APP_WEBHOOK_SECRET }} | |
| GITHUB_CLIENT_ID: ${{ secrets.GITHUB_CLIENT_ID }} | |
| GITHUB_CLIENT_SECRET: ${{ secrets.GITHUB_CLIENT_SECRET }} |
| GITHUB_CLIENT_ID: fake-dashboard-client-id | ||
| GITHUB_CLIENT_SECRET: fake-dashboard-client-secret | ||
| AUTH_CALLBACK_URL: https://codra.test/auth/github/callback | ||
| APP_URL: https://codra.test |
There was a problem hiding this comment.
The DASHBOARD_ALLOWED_USERS environment variable contains a specific username (devarshishimpi). For better maintainability and to avoid using real user identities in CI/CD configurations, consider using a generic test user identifier.
| APP_URL: https://codra.test | |
| DASHBOARD_ALLOWED_USERS: test-user |
| APP_URL: requiredEnv('APP_URL'), | ||
| DASHBOARD_ALLOWED_USERS: requiredEnv('DASHBOARD_ALLOWED_USERS'), | ||
| get GEMINI_API_KEY() { return unusedEnv('GEMINI_API_KEY'); }, | ||
| BOT_USERNAME: requiredEnv('BOT_USERNAME'), |
There was a problem hiding this comment.
The 'ENVIRONMENT' property in 'createTestEnv' has been changed from a hardcoded 'test' value to a getter that calls 'unusedEnv()', which throws an Error. If any application code or test helper relies on the default 'ENVIRONMENT' being 'test' without providing it in the 'overrides' object, the tests will crash at runtime.
| BOT_USERNAME: requiredEnv('BOT_USERNAME'), | |
| ENVIRONMENT: 'test', |
| ENVIRONMENT: 'test', | ||
| CF_API_TOKEN: 'cf-api-token', | ||
| CF_ACCOUNT_ID: 'cf-account-id', | ||
| CF_DLQ_ID: 'cf-dlq-id', |
There was a problem hiding this comment.
Properties like 'APP_PRIVATE_KEY' were previously provided with dummy values. They are now getters that throw via 'unusedEnv()'. If the system under test performs validation on these keys during bootstrap (e.g., checking for a valid PEM format), the tests will fail immediately upon initialization unless these keys are explicitly provided in the environment or overrides.
| @@ -10,19 +23,25 @@ vi.stubGlobal('QUEUE', { | |||
There was a problem hiding this comment.
The QUEUE stub function uses any for the message parameter. This reduces type safety and makes the code harder to maintain or refactor. It is better to define an interface for the expected message structure.
| @@ -10,19 +23,25 @@ vi.stubGlobal('QUEUE', { | |
| interface QueueMessage { | |
| [key: string]: unknown; | |
| } | |
| // ... inside vi.stubGlobal | |
| send: async (msg: QueueMessage) => { ... } |
| return trimmed; | ||
| return trimmed.replace(/\\n/g, '\n'); | ||
| } | ||
|
|
There was a problem hiding this comment.
The usableEnvValue function explicitly returns null if the value is the string 'undefined' or 'null'. While this likely prevents accidental string values from being passed to the database driver, it might mask a legitimate test case where these strings are the intended value.
| function usableEnvValue(value: string | undefined) { | |
| // Explicitly check for the string literals to prevent them from being treated as missing, | |
| // but add a comment explaining this specific behavior. | |
| return value && value !== 'undefined' && value !== 'null' ? value : null; | |
| } |
| return value && value !== 'undefined' && value !== 'null' ? value : null; | ||
| } | ||
|
|
||
| function loadEnvFiles() { |
There was a problem hiding this comment.
The loadEnvFiles function implements a custom parser for .env files by splitting content by newlines and slicing at the first '='. This approach is fragile and fails to handle several common .env standards: 1) Inline comments (e.g., 'KEY=VALUE # comment' will result in the value being 'VALUE # comment'), 2) Multi-line quoted values (the line-by-line split breaks these), and 3) Quote stripping fails if there is any trailing whitespace or comments after the closing quote. Using a battle-tested library like 'dotenv' is recommended, or the parser should be enhanced to handle these edge cases.
| function loadEnvFiles() { | |
| // Consider using a library like dotenv or improving the regex: | |
| // Example for basic inline comment removal: | |
| // const lineWithoutComment = line.split('#')[0].trim(); |
| } | ||
|
|
||
| function usableEnvValue(value) { | ||
| return value && value !== 'undefined' && value !== 'null' ? value : null; |
There was a problem hiding this comment.
The usableEnvValue function uses a truthiness check (value && ...), which means an empty string ("") will be returned as null. While likely acceptable for database URLs, this pattern can cause bugs if other environment variables are loaded where an empty string is a distinct and valid configuration value different from null or undefined.
| return value && value !== 'undefined' && value !== 'null' ? value : null; | |
| return (value !== undefined && value !== null && value !== 'undefined' && value !== 'null') ? value : null; |
1 participant
Description
Sets up CI to run the full Codra test suite on PRs, pushes to main, and manual workflow dispatches.
Changes include:
.env.test.examplefor local test setup..dev.vars.exampleto match the local.dev.varsenv names.Closes #1
Type of change
How Has This Been Tested?
Checklist: