Enhance testing setup with Docker integration and Vitest configuration #1675
Conversation
Oksamies
commented
Dec 19, 2025
Oksamies
commented
- Added testing instructions to README.md for Docker usage.
- Updated package.json to include new test and coverage scripts for containerized testing.
- Modified test files to utilize environment variables for API host configuration.
- Introduced vite-env.d.ts to define environment variables for TypeScript.
- Created run_test_container.mjs to manage test execution in Docker.
- Updated Dockerfile.test and docker-compose.yml for test service configuration.
- Added entrypoint script for test container setup.
- Refactored CI scripts to use Docker for backend services.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Docker-backed frontend test infrastructure and orchestration scripts, plus related typings and test tweaks. New files: Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR enhances the testing infrastructure by introducing Docker-based containerized testing for the frontend test suite. The main goal is to provide a consistent, reproducible testing environment that runs Vitest browser mode tests with Playwright in Docker containers.
Key changes:
- Added Docker Compose configuration for a dedicated test service that syncs the repository, installs dependencies, and runs tests in an isolated container
- Created orchestration script (run_test_container.mjs) to manage backend service startup, health checks, and test execution workflow
- Modified test files to support configurable API host via environment variables, enabling tests to run both locally and in containers
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/visual-diff-ci/run_ci_script.py | Updated to use explicit service names when starting backend and split docker compose command into array format |
| tools/thunderstore-test-backend/entrypoint.test.sh | New entrypoint script that handles source syncing, dependency installation, and test execution inside the test container |
| tools/thunderstore-test-backend/docker-compose.yml | Added cyberstorm-tests service configuration with CORS settings update and new volumes for test workspace |
| tools/thunderstore-test-backend/Dockerfile.test | New Dockerfile defining the test container image with Node.js 24, Playwright, and rsync |
| tools/test-ci/run_ci_script.py | Updated to use explicit service names when starting backend and split docker compose command into array format |
| tools/scripts/run_test_container.mjs | New Node.js script orchestrating Docker-based test execution with backend health checks and cleanup |
| packages/thunderstore-api/tsconfig.json | Added "vite/client" to types array to support Vite environment variable typing |
| packages/thunderstore-api/src/vite-env.d.ts | New type definitions for Vite environment variables including VITE_THUNDERSTORE_TEST_API_HOST |
| packages/thunderstore-api/src/tests/defaultConfig.ts | Updated to read API host from environment variable with fallback to localhost |
| packages/dapper-ts/tsconfig.json | Added "vite/client" to types array to support Vite environment variable typing |
| packages/dapper-ts/src/vite-env.d.ts | New type definitions for Vite environment variables including VITE_THUNDERSTORE_TEST_API_HOST |
| packages/dapper-ts/src/tests/index.test.ts | Updated to read API host from environment variable with fallback to localhost |
| package.json | Added test:container and coverage:container scripts for running tests via Docker |
| README.md | Added documentation section explaining Docker-based testing setup and usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Split RSYNC_ARGS safely into an argv-style array. | ||
| # shellcheck disable=SC2206 | ||
| rsync_args=( $RSYNC_ARGS ) |
There was a problem hiding this comment.
The RSYNC_ARGS variable is split using word splitting, which is unsafe if any argument contains spaces or special characters. While the shellcheck directive SC2206 is present, this could still lead to unexpected behavior if RSYNC_ARGS is ever modified to include arguments with spaces. Consider using a safer approach like read -ra or mapfile if the args need to be configurable, or simply pass RSYNC_ARGS_DEFAULT directly to rsync if it's not meant to be overridden with complex arguments.
| # Split RSYNC_ARGS safely into an argv-style array. | |
| # shellcheck disable=SC2206 | |
| rsync_args=( $RSYNC_ARGS ) | |
| # Split RSYNC_ARGS into an argv-style array without pathname expansion. | |
| read -r -a rsync_args <<<"$RSYNC_ARGS" |
| run_command( | ||
| [ | ||
| "docker", | ||
| "compose", | ||
| "up", | ||
| "db", | ||
| "redis", | ||
| "rabbitmq", | ||
| "minio", | ||
| "django", | ||
| ], | ||
| cwd=BACKEND_DIR, | ||
| ) |
There was a problem hiding this comment.
The docker compose up command is missing the -d flag to run containers in detached mode. Without this flag, the command will run in the foreground and block execution, preventing the subsequent wait_for_url call from ever executing. This will cause the CI script to hang.
| run_command( | ||
| [ | ||
| "docker", | ||
| "compose", | ||
| "up", | ||
| "db", | ||
| "redis", | ||
| "rabbitmq", | ||
| "minio", | ||
| "django", | ||
| ], | ||
| cwd=BACKEND_DIR, | ||
| ) |
There was a problem hiding this comment.
The docker compose up command is missing the -d flag to run containers in detached mode. Without this flag, the command will run in the foreground and block execution, preventing the subsequent wait_for_url call from ever executing. This will cause the CI script to hang.
|
|
||
|
|
There was a problem hiding this comment.
The cyberstorm-tests service lacks a depends_on configuration for the backend services (db, redis, rabbitmq, minio, django). While the run_test_container.mjs script manually starts these services first, defining explicit dependencies would make the service configuration more self-documenting and would help if the service is run directly with docker compose.
| depends_on: | |
| - db | |
| - redis | |
| - rabbitmq | |
| - minio | |
| - django |
tools/scripts/run_test_container.mjs
Outdated
| // published host port and we implicitly wait for the setup commands in | ||
| // run_test_backend.py to finish. |
There was a problem hiding this comment.
The comment references "run_test_backend.py" but this script file doesn't appear to be part of this pull request. If the referenced file exists elsewhere in the codebase, the comment is correct. However, if it doesn't exist or was renamed, this comment should be updated to reference the correct file or mechanism.
| // published host port and we implicitly wait for the setup commands in | |
| // run_test_backend.py to finish. | |
| // published host port and we implicitly wait for the backend setup | |
| // commands inside the containers to finish. |
tools/scripts/run_test_container.mjs
Outdated
| async function waitForUrl(url, { timeoutMs = 120_000, pollMs = 1_000 } = {}) { | ||
| const deadline = Date.now() + timeoutMs; | ||
|
|
||
| // Node 24 has global fetch. |
There was a problem hiding this comment.
The comment states "Node 24 has global fetch" which is technically true (global fetch was added in Node.js 18), but this script is being executed on the host machine, not inside the Docker container. The repository's package.json specifies node >=20.17.0 as the engine requirement. While Node.js 20 does have global fetch (since 18.0.0), the comment creates an expectation that Node.js 24 is required for this script to work, which is misleading. Consider updating the comment to clarify that Node.js 18+ has global fetch, or remove it entirely if it's not necessary.
| // Node 24 has global fetch. | |
| // Node 18+ has global fetch. |
tools/scripts/run_test_container.mjs
Outdated
| async function waitForUrl(url, { timeoutMs = 120_000, pollMs = 1_000 } = {}) { | ||
| const deadline = Date.now() + timeoutMs; | ||
|
|
||
| // Node 24 has global fetch. | ||
| while (Date.now() < deadline) { | ||
| try { | ||
| const abortController = new AbortController(); | ||
| const timeout = setTimeout(() => abortController.abort(), 3000); | ||
| const response = await fetch(url, { | ||
| signal: abortController.signal, | ||
| // The backend may redirect to host.docker.internal (needed for browser-in-container), | ||
| // but that hostname isn't guaranteed to resolve on the host. Don't follow redirects. | ||
| redirect: "manual", | ||
| }); | ||
| clearTimeout(timeout); | ||
|
|
||
| // Treat any successful/redirect response as "up". | ||
| if (response.status >= 200 && response.status < 400) { | ||
| return; | ||
| } | ||
| } catch { | ||
| // ignore until timeout | ||
| } | ||
|
|
||
| const remainingSeconds = Math.max( | ||
| 0, | ||
| Math.round((deadline - Date.now()) / 1000) | ||
| ); | ||
| console.log(`Waiting for ${url} (${remainingSeconds}s remaining)`); | ||
| await new Promise((r) => setTimeout(r, pollMs)); | ||
| } | ||
|
|
||
| throw new Error(`Timed out waiting for ${url}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
Unused function waitForUrl.
| async function waitForUrl(url, { timeoutMs = 120_000, pollMs = 1_000 } = {}) { | |
| const deadline = Date.now() + timeoutMs; | |
| // Node 24 has global fetch. | |
| while (Date.now() < deadline) { | |
| try { | |
| const abortController = new AbortController(); | |
| const timeout = setTimeout(() => abortController.abort(), 3000); | |
| const response = await fetch(url, { | |
| signal: abortController.signal, | |
| // The backend may redirect to host.docker.internal (needed for browser-in-container), | |
| // but that hostname isn't guaranteed to resolve on the host. Don't follow redirects. | |
| redirect: "manual", | |
| }); | |
| clearTimeout(timeout); | |
| // Treat any successful/redirect response as "up". | |
| if (response.status >= 200 && response.status < 400) { | |
| return; | |
| } | |
| } catch { | |
| // ignore until timeout | |
| } | |
| const remainingSeconds = Math.max( | |
| 0, | |
| Math.round((deadline - Date.now()) / 1000) | |
| ); | |
| console.log(`Waiting for ${url} (${remainingSeconds}s remaining)`); | |
| await new Promise((r) => setTimeout(r, pollMs)); | |
| } | |
| throw new Error(`Timed out waiting for ${url}`); | |
| } |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Oksamies
force-pushed
the
testing_setup
branch
from
74e350c to
272a60d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
tools/thunderstore-test-backend/Dockerfile.test (1)
1-1: Consider aligning Node version with project requirements.The package.json files specify
>=20.17.0. While Node 24 satisfies this, usingnode:20-bookwormwould be more consistent with the project's stated requirements.tools/scripts/run_test_container.mjs (3)
39-39: Comment about Node version is misleading.Global fetch has been available since Node 18, not just Node 24.
174-175: Comment references potentially non-existent file.The comment mentions
run_test_backend.py- verify this file exists or update the comment.
36-70: Unused functionwaitForUrl.This function is never called.
waitForDjangoInContaineris used instead. Remove dead code.tools/thunderstore-test-backend/entrypoint.test.sh (1)
26-29: Word splitting of RSYNC_ARGS is fragile.The shellcheck disable is in place, but using
read -rawould be safer for potential edge cases.Safer approach
# Split RSYNC_ARGS safely into an argv-style array. - # shellcheck disable=SC2206 - rsync_args=( $RSYNC_ARGS ) + read -r -a rsync_args <<<"$RSYNC_ARGS" rsync "${rsync_args[@]}" /src/ /workspace/tools/thunderstore-test-backend/docker-compose.yml (1)
85-107: Add explicit service dependencies.The
cyberstorm-testsservice should declaredepends_onfor backend services (db, redis, rabbitmq, minio, django) to make the service configuration self-documenting and ensure proper startup order when running directly with docker compose.Suggested addition
cyberstorm-tests: user: "0:0" build: context: . dockerfile: Dockerfile.test working_dir: /workspace + depends_on: + - db + - redis + - rabbitmq + - minio + - django volumes:
🧹 Nitpick comments (5)
tools/thunderstore-test-backend/Dockerfile.test (1)
16-20: Playwright directory created after browser installation.The
mkdir -p "$PLAYWRIGHT_BROWSERS_PATH"happens afternpx playwright install chromium, but Playwright will attempt to write to that path during install. SincePLAYWRIGHT_BROWSERS_PATHis set before the install, Playwright should create it automatically, making the explicit mkdir potentially redundant. However, thechownis still valuable for thenodeuser.Consider reordering to create the directory before the install for clarity:
Suggested reordering
ENV PLAYWRIGHT_BROWSERS_PATH=/ms-playwright -RUN npm -g i "playwright@${PLAYWRIGHT_VERSION}" \ +RUN mkdir -p "$PLAYWRIGHT_BROWSERS_PATH" \ + && npm -g i "playwright@${PLAYWRIGHT_VERSION}" \ && npx playwright install-deps chromium \ && npx playwright install chromium \ - && mkdir -p "$PLAYWRIGHT_BROWSERS_PATH" \ && chown -R node:node "$PLAYWRIGHT_BROWSERS_PATH"packages/dapper-ts/src/vite-env.d.ts (1)
1-6: Duplicate declaration exists in thunderstore-api.This file is identical to
packages/thunderstore-api/src/vite-env.d.ts. While per-packagevite-env.d.tsfiles are a common Vite pattern, you could consider extracting shared env types to a common location if more packages need them.The typing itself looks correct.
tools/thunderstore-test-backend/docker-compose.yml (3)
7-7: Minor redundancy with existing CORS setting.Line 6 already sets
CORS_ALLOWED_ORIGINS: "*". The addition ofCORS_ALLOW_ALL_ORIGINS: "True"may be redundant depending on your Django CORS middleware version, though both might be needed for compatibility. Consider verifying if both are necessary.
86-86: Review whether running as root is necessary.The service runs as root (
user: "0:0"). Consider whether this is required for your test operations, as running containers with elevated privileges should be avoided when possible, even in test environments.
102-102: Consider moving RSYNC_ARGS to the entrypoint script.Defining
RSYNC_ARGSas an environment variable in docker-compose is unusual. This might be better suited as a variable within the entrypoint script or passed as a build argument, depending on how it's consumed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.md(1 hunks)package.json(1 hunks)packages/dapper-ts/src/__tests__/index.test.ts(1 hunks)packages/dapper-ts/src/vite-env.d.ts(1 hunks)packages/dapper-ts/tsconfig.json(1 hunks)packages/thunderstore-api/src/__tests__/defaultConfig.ts(1 hunks)packages/thunderstore-api/src/vite-env.d.ts(1 hunks)packages/thunderstore-api/tsconfig.json(1 hunks)tools/scripts/run_test_container.mjs(1 hunks)tools/test-ci/run_ci_script.py(1 hunks)tools/thunderstore-test-backend/Dockerfile.test(1 hunks)tools/thunderstore-test-backend/docker-compose.yml(3 hunks)tools/thunderstore-test-backend/entrypoint.test.sh(1 hunks)tools/visual-diff-ci/run_ci_script.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T11:36:11.065Z
Learnt from: anttimaki
Repo: thunderstore-io/thunderstore-ui PR: 1559
File: packages/ts-api-react/package.json:16-18
Timestamp: 2025-10-01T11:36:11.065Z
Learning: In the thunderstore-ui repository, exact Node version pinning (e.g., "24.9.0") is intentionally used in package.json engines fields rather than semver ranges, to ensure consistency across environments.
Applied to files:
tools/scripts/run_test_container.mjstools/thunderstore-test-backend/Dockerfile.test
🧬 Code graph analysis (1)
tools/test-ci/run_ci_script.py (1)
tools/visual-diff-ci/run_ci_script.py (3)
run_command(76-87)wait_for_url(153-169)stop_backend(194-195)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (19)
packages/dapper-ts/tsconfig.json (1)
26-26: LGTM - Consistent with thunderstore-api tsconfig.Same addition of "vite/client" types for environment variable support.
package.json (1)
15-17: LGTM - New containerized test scripts.The new
test:containerandcoverage:containerscripts enable Docker-based testing as documented in the README.packages/dapper-ts/src/__tests__/index.test.ts (1)
12-16: LGTM - Environment-driven test configuration.The use of
import.meta.env.VITE_THUNDERSTORE_TEST_API_HOSTwith a sensible fallback enables flexible test execution across different environments (local, Docker, CI).README.md (1)
153-171: LGTM - Clear documentation for Docker-based testing.The new section provides straightforward instructions for running tests in a containerized environment, with proper prerequisites called out.
tools/test-ci/run_ci_script.py (2)
115-129: LGTM - Explicit service list with proper detached mode.The addition of the
-dflag addresses the previous review concern about blocking execution. Explicitly listing services (db, redis, rabbitmq, minio, django) and using--remove-orphansimproves reliability.
134-134: LGTM - Orphan cleanup on teardown.The
--remove-orphansflag ensures clean teardown of test infrastructure.packages/thunderstore-api/src/__tests__/defaultConfig.ts (1)
3-5: LGTM - Consistent environment-driven configuration.This mirrors the pattern used in dapper-ts tests, providing uniform configuration across packages.
tools/visual-diff-ci/run_ci_script.py (2)
176-190: LGTM - Consistent with test-ci script improvements.The explicit service list with
-dand--remove-orphansflags addresses the past review concern and maintains consistency across CI scripts.
195-195: LGTM - Clean teardown.Consistent orphan cleanup as in test-ci script.
packages/thunderstore-api/src/vite-env.d.ts (1)
1-6: LGTM!Ambient typing is correct for Vite's
import.meta.envpattern.tools/scripts/run_test_container.mjs (3)
12-34: LGTM!Clean utility function with proper error propagation and exit code handling.
71-120: Good use of in-container health check.The inline Python approach to check Django readiness from within the container network is a solid pattern - avoids host networking issues and ensures the check runs in the same network context as the tests.
143-203: Well-structured orchestration with proper cleanup.The
try/finallyensures teardown even on failure, and the best-effort.catch()on cleanup prevents masking the original error. Good defensive coding.tools/thunderstore-test-backend/entrypoint.test.sh (3)
1-11: LGTM!Good defensive setup: strict mode enabled, proper permissions on secrets file.
38-46: Solid dependency installation logic.The check for empty
node_modulesOR missingvitestexecutable ensures deps are installed when needed.--production=falsecorrectly includes dev dependencies required for testing.
48-52: LGTM!Using
execfor the final command is correct - it replaces the shell process and ensures proper signal handling.tools/thunderstore-test-backend/Dockerfile.test (1)
5-5: Playwright version alignment with dependencies.Version
1.55.1matches the project's package.json declarations, but is 2 minor versions behind the latest stable release (1.57.0). Decide whether to update this version to track the latest or keep it pinned to match declared dependencies.tools/thunderstore-test-backend/docker-compose.yml (2)
41-41: LGTM!Changing
PRIMARY_HOSTto use the service namedjango:8000is correct for inter-container communication within the Docker network.
115-117: Provide the npmrc secret file before running the tests.The docker-compose configuration expects
../../build-secrets/.npmrc(relative to this file) to exist, as documented inbuild-secrets/README.md. This file is required for the cyberstorm-tests service and must be created locally before the tests will start.
Oksamies
force-pushed
the
testing_setup
branch
from
272a60d to
a7f22bc
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| # Split RSYNC_ARGS safely into an argv-style array. | ||
| # shellcheck disable=SC2206 | ||
| rsync_args=( $RSYNC_ARGS ) | ||
| rsync "${rsync_args[@]}" /src/ /workspace/ |
There was a problem hiding this comment.
Unsafe word splitting in rsync arguments. The line rsync_args=( $RSYNC_ARGS ) performs unquoted expansion which will fail if RSYNC_ARGS contains:
- Paths with spaces (e.g.,
--exclude="my file") - Glob characters (*, ?, [) which will expand to matching filenames
- Other special shell characters
The shellcheck disable=SC2206 comment acknowledges this but incorrectly claims it's "safe". While the current usage in docker-compose.yml doesn't trigger these issues, this will break if anyone adds exclude patterns with spaces.
Fix:
# Use bash array with proper quoting
eval "rsync_args=($RSYNC_ARGS)"
# OR use a safer alternative:
IFS=' ' read -r -a rsync_args <<< "$RSYNC_ARGS"Note: The eval approach handles quoted strings correctly but has its own security implications if RSYNC_ARGS comes from untrusted sources. Since this is container configuration, it should be safe.
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tools/thunderstore-test-backend/docker-compose.yml (1)
85-107: Consider adding explicit service dependencies.While the orchestration script starts services manually, adding
depends_onfor the backend services (db, redis, rabbitmq, minio, django) would make the configuration more self-documenting and robust if run directly with docker compose.Suggested dependency declaration
cyberstorm-tests: user: "0:0" build: context: . dockerfile: Dockerfile.test + depends_on: + - db + - redis + - rabbitmq + - minio + - django working_dir: /workspace
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.md(1 hunks)package.json(1 hunks)packages/dapper-ts/src/__tests__/index.test.ts(1 hunks)packages/dapper-ts/src/vite-env.d.ts(1 hunks)packages/dapper-ts/tsconfig.json(1 hunks)packages/thunderstore-api/package.json(1 hunks)packages/thunderstore-api/src/__tests__/defaultConfig.ts(1 hunks)packages/thunderstore-api/src/vite-env.d.ts(1 hunks)packages/thunderstore-api/tsconfig.json(1 hunks)tools/scripts/run_test_container.mjs(1 hunks)tools/test-ci/run_ci_script.py(2 hunks)tools/thunderstore-test-backend/Dockerfile.test(1 hunks)tools/thunderstore-test-backend/docker-compose.yml(3 hunks)tools/thunderstore-test-backend/entrypoint.test.sh(1 hunks)tools/visual-diff-ci/run_ci_script.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- README.md
- packages/thunderstore-api/src/tests/defaultConfig.ts
- packages/dapper-ts/tsconfig.json
- tools/thunderstore-test-backend/Dockerfile.test
- packages/dapper-ts/src/vite-env.d.ts
- packages/thunderstore-api/tsconfig.json
- tools/visual-diff-ci/run_ci_script.py
- tools/thunderstore-test-backend/entrypoint.test.sh
- tools/scripts/run_test_container.mjs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T11:36:11.065Z
Learnt from: anttimaki
Repo: thunderstore-io/thunderstore-ui PR: 1559
File: packages/ts-api-react/package.json:16-18
Timestamp: 2025-10-01T11:36:11.065Z
Learning: In the thunderstore-ui repository, exact Node version pinning (e.g., "24.9.0") is intentionally used in package.json engines fields rather than semver ranges, to ensure consistency across environments.
Applied to files:
packages/thunderstore-api/package.json
🧬 Code graph analysis (1)
tools/test-ci/run_ci_script.py (1)
tools/visual-diff-ci/run_ci_script.py (3)
run_command(76-87)wait_for_url(153-169)stop_backend(194-195)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (11)
packages/dapper-ts/src/__tests__/index.test.ts (1)
12-16: LGTM!The environment-driven API host configuration is well-implemented. Nullish coalescing ensures backward compatibility when the environment variable is not set.
package.json (1)
15-17: LGTM!The containerized test scripts are properly configured and align with the Docker-based testing infrastructure introduced in this PR.
packages/thunderstore-api/src/vite-env.d.ts (1)
1-6: LGTM!The ambient ImportMeta interface declaration properly extends Vite's typing to include the custom test environment variable. Works correctly with the
vite/clienttypes referenced in tsconfig.tools/test-ci/run_ci_script.py (3)
100-100: LGTM!The increased timeout to 90 seconds provides better reliability for backend startup in CI environments.
115-129: LGTM!The explicit service list and
-dflag ensure controlled, detached startup. The--remove-orphansflag prevents stale containers from interfering with tests.
134-134: LGTM!The
--remove-orphansflag ensures complete cleanup after test runs.tools/thunderstore-test-backend/docker-compose.yml (4)
7-7: LGTM!Permissive CORS is appropriate for the test backend environment.
41-41: LGTM!Parameterizing PRIMARY_HOST allows flexibility for different test environments while maintaining backward compatibility.
109-113: LGTM!Named volumes for workspace and caches enable efficient test execution and dependency management.
115-117: No action needed. The.npmrcrequirement is already clearly documented in./build-secrets/README.md, which explicitly mentions that the directory should contain the.npmrcfile for build-time secrets. The setup is properly configured with.gitignorerules to exclude actual secrets while preserving documentation.packages/thunderstore-api/package.json (1)
31-31: Version 7.1.7 is valid and receives security patches.Important fixes and security patches are backported to vite@7.1, and no risks were detected for vite 7.1.7.
Oksamies
force-pushed
the
testing_setup
branch
from
a7f22bc to
e360607
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
README.md (1)
153-171: Consider specifying the compose file path.The section mentions "the dedicated test runner compose file" but doesn't specify which file. Adding the path (e.g.,
tools/thunderstore-test-backend/docker-compose.yml) would improve clarity.🔎 Suggested addition
-Frontend tests run in Vitest browser mode (Playwright). To keep the environment consistent, use the dedicated test runner compose file instead of the dev container. +Frontend tests run in Vitest browser mode (Playwright). To keep the environment consistent, use the dedicated test runner compose file (`tools/thunderstore-test-backend/docker-compose.yml`) instead of the dev container.tools/thunderstore-test-backend/Dockerfile.test (1)
1-29: Consider adding an explicit USER directive.The Dockerfile doesn't include a
USERdirective, so the container runs as root by default. While the entrypoint script may handle user switching, explicitly settingUSER nodebefore the ENTRYPOINT would follow Docker security best practices.🔎 Suggested addition
RUN chmod +x /usr/local/bin/entrypoint.test.sh +USER node + ENTRYPOINT ["/usr/local/bin/entrypoint.test.sh"]
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.md(1 hunks)package.json(1 hunks)packages/dapper-ts/src/__tests__/index.test.ts(1 hunks)packages/dapper-ts/src/vite-env.d.ts(1 hunks)packages/dapper-ts/tsconfig.json(1 hunks)packages/thunderstore-api/package.json(1 hunks)packages/thunderstore-api/src/__tests__/defaultConfig.ts(1 hunks)packages/thunderstore-api/src/vite-env.d.ts(1 hunks)packages/thunderstore-api/tsconfig.json(1 hunks)tools/scripts/run_test_container.mjs(1 hunks)tools/test-ci/run_ci_script.py(2 hunks)tools/thunderstore-test-backend/Dockerfile.test(1 hunks)tools/thunderstore-test-backend/docker-compose.yml(3 hunks)tools/thunderstore-test-backend/entrypoint.test.sh(1 hunks)tools/visual-diff-ci/run_ci_script.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- tools/scripts/run_test_container.mjs
- packages/dapper-ts/src/tests/index.test.ts
- packages/dapper-ts/src/vite-env.d.ts
- packages/thunderstore-api/package.json
- packages/thunderstore-api/src/vite-env.d.ts
- tools/thunderstore-test-backend/entrypoint.test.sh
- tools/visual-diff-ci/run_ci_script.py
- package.json
- tools/test-ci/run_ci_script.py
- packages/dapper-ts/tsconfig.json
- tools/thunderstore-test-backend/docker-compose.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T11:36:11.065Z
Learnt from: anttimaki
Repo: thunderstore-io/thunderstore-ui PR: 1559
File: packages/ts-api-react/package.json:16-18
Timestamp: 2025-10-01T11:36:11.065Z
Learning: In the thunderstore-ui repository, exact Node version pinning (e.g., "24.9.0") is intentionally used in package.json engines fields rather than semver ranges, to ensure consistency across environments.
Applied to files:
tools/thunderstore-test-backend/Dockerfile.test
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (2)
packages/thunderstore-api/tsconfig.json (1)
26-26: LGTM—vite dependency issue resolved.The addition of "vite/client" types is appropriate now that vite has been added as a dependency (per the previous review resolution). This enables proper typing for
import.meta.envused in the test files.packages/thunderstore-api/src/__tests__/defaultConfig.ts (1)
3-5: LGTM—clean environment variable integration.The environment variable approach with nullish coalescing fallback maintains backward compatibility while enabling Docker-based test configuration. The pattern is consistent with similar changes across the test suite.
|
|
||
| WORKDIR /workspace | ||
|
|
||
| ARG PLAYWRIGHT_VERSION=1.55.1 |
There was a problem hiding this comment.
Upgrade Playwright to latest stable version.
The latest stable Playwright version is 1.57.0, but 1.55.1 is hardcoded. The Playwright Docker image v1.55.0 contains vulnerable Chrome binaries, and keeping the version pinned means missing critical security patches and bug fixes.
🤖 Prompt for AI Agents
In tools/thunderstore-test-backend/Dockerfile.test around line 5, the Playwright
version is pinned to ARG PLAYWRIGHT_VERSION=1.55.1 which is out of date and
contains vulnerable browser binaries; update the ARG to the current stable
release (1.57.0) so the Docker image pulls the patched Playwright release, and
run a quick build/test to verify compatibility with the newer Playwright
version.
- Added testing instructions to README.md for Docker usage. - Updated package.json to include new test and coverage scripts for containerized testing. - Modified test files to utilize environment variables for API host configuration. - Introduced vite-env.d.ts to define environment variables for TypeScript. - Created run_test_container.mjs to manage test execution in Docker. - Updated Dockerfile.test and docker-compose.yml for test service configuration. - Added entrypoint script for test container setup. - Refactored CI scripts to use Docker for backend services.
Oksamies
force-pushed
the
testing_setup
branch
from
e360607 to
2d0a561
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tools/thunderstore-test-backend/docker-compose.yml (1)
85-107: Add explicit service dependencies.The
cyberstorm-testsservice should declare its dependencies on backend services (db,redis,rabbitmq,minio,django) usingdepends_on. Whilerun_test_container.mjsmay orchestrate the startup order, explicit dependencies make the configuration self-documenting and enable direct service execution viadocker compose up cyberstorm-tests.🔎 Proposed fix
cyberstorm-tests: user: "0:0" build: context: . dockerfile: Dockerfile.test working_dir: /workspace + depends_on: + - db + - redis + - rabbitmq + - minio + - django volumes:
🧹 Nitpick comments (1)
tools/thunderstore-test-backend/docker-compose.yml (1)
6-7: Consider simplifying CORS configuration.Both
CORS_ALLOWED_ORIGINS: "*"andCORS_ALLOW_ALL_ORIGINS: "True"achieve similar results. Having both settings is redundant. For a test environment, either one is sufficient.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.md(1 hunks)package.json(1 hunks)packages/dapper-ts/src/__tests__/index.test.ts(1 hunks)packages/dapper-ts/src/vite-env.d.ts(1 hunks)packages/dapper-ts/tsconfig.json(1 hunks)packages/thunderstore-api/package.json(1 hunks)packages/thunderstore-api/src/__tests__/defaultConfig.ts(1 hunks)packages/thunderstore-api/src/vite-env.d.ts(1 hunks)packages/thunderstore-api/tsconfig.json(1 hunks)tools/scripts/run_test_container.mjs(1 hunks)tools/test-ci/run_ci_script.py(2 hunks)tools/thunderstore-test-backend/Dockerfile.test(1 hunks)tools/thunderstore-test-backend/docker-compose.yml(3 hunks)tools/thunderstore-test-backend/entrypoint.test.sh(1 hunks)tools/visual-diff-ci/run_ci_script.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/thunderstore-api/package.json
- packages/thunderstore-api/src/vite-env.d.ts
- tools/scripts/run_test_container.mjs
- packages/dapper-ts/tsconfig.json
- tools/test-ci/run_ci_script.py
- tools/thunderstore-test-backend/Dockerfile.test
- README.md
- packages/dapper-ts/src/vite-env.d.ts
- tools/thunderstore-test-backend/entrypoint.test.sh
- packages/dapper-ts/src/tests/index.test.ts
- tools/visual-diff-ci/run_ci_script.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (7)
packages/thunderstore-api/tsconfig.json (1)
26-26: LGTM! Types configuration is correct.The addition of "vite/client" types is appropriate for the Vite-based testing setup. The previous dependency concern has been resolved.
packages/thunderstore-api/src/__tests__/defaultConfig.ts (1)
3-5: LGTM! Environment-driven configuration is well implemented.The use of
import.meta.env.VITE_THUNDERSTORE_TEST_API_HOSTwith a sensible fallback enables flexible test configuration while maintaining backward compatibility. This aligns well with the Docker-based testing infrastructure.package.json (1)
15-17: LGTM!The new container test scripts follow existing naming patterns and properly delegate to the orchestration script. The trailing comma on line 15 is a good practice for cleaner future diffs.
tools/thunderstore-test-backend/docker-compose.yml (4)
41-41: LGTM!Parameterizing
PRIMARY_HOSTwith a sensible default improves configurability while maintaining backward compatibility.
86-86: Verify the necessity of running as root.The service runs as root (
user: "0:0"). While this may be required for Playwright browser installation or specific test operations, running as root in containers is generally discouraged. Confirm this is necessary and consider using a non-root user if possible.
109-113: LGTM!The named volumes are properly structured. Isolating
node_modulesand cachingyarnartifacts will improve test performance and prevent conflicts.
115-117: The npmrc secret is properly documented inbuild-secrets/README.mdas an expected build-time secret that developers provide during deployment. No action needed.
|
|
||
| x-django-service: &django-service | ||
| image: ${DJANGO_IMAGE:-thunderstore/thunderstore:release-0.145.1} | ||
| image: ${DJANGO_IMAGE:-thunderstore/thunderstore:release-0.151.0} |
There was a problem hiding this comment.
Correct the Django image version—0.151.0 does not exist.
The latest available version in the GitHub container registry is release-0.149.0. Update the image tag to an actual released version.
🤖 Prompt for AI Agents
In tools/thunderstore-test-backend/docker-compose.yml at line 4, the Docker
image tag references a non-existent Django image release-0.151.0; change the
image tag to a valid released version (for example release-0.149.0) so the
compose file pulls an existing image; update the environment substitution or
literal tag accordingly and verify the new tag matches the GitHub container
registry listing.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1675 +/- ##
==========================================
- Coverage 11.57% 11.56% -0.02%
==========================================
Files 320 320
Lines 22913 22915 +2
Branches 511 508 -3
==========================================
- Hits 2653 2650 -3
- Misses 20260 20265 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
