feat(integration-tests): Add end-to-end test workflow for downstream clp-ffi-js; Bump yscope-dev-utils to y-scope/yscope-dev-utils@e2a1aed.#2015
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new taskfile for clp-ffi-js that downloads, extracts, symlinks yscope-dev-utils, builds and runs Node and browser tests; wires that taskfile into main test includes; and updates the yscope-dev-utils downloader invocation and submodule commit SHA. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant DevMachine as Dev Machine
participant Downloader as Downloader Script
participant Repo as Workspace Repo
participant Yscope as tools/yscope-dev-utils
participant CMake as CMake Build
participant NodeTest as Node Test Runner
participant BrowserTest as Browser Test Runner
DevMachine->>Downloader: run deps-download/init.sh (url, dest, --extract)
Downloader->>Repo: extract clp-ffi-js into build/clp-ffi-js
DevMachine->>Repo: run `task default` (clp-ffi-js)
Repo->>Yscope: create symlink clp-ffi-js/.../tools/yscope-dev-utils → workspace/tools/yscope-dev-utils
Repo->>CMake: build ClpFfiJs-node
CMake->>NodeTest: provide Node module path
NodeTest->>DevMachine: run Node tests (npm test with VITE_NODE_MODULE_ABS_PATH)
Repo->>CMake: build ClpFfiJs-worker (requires sudo)
CMake->>BrowserTest: provide Worker module path and init tests
BrowserTest->>DevMachine: run browser tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@taskfiles/tests/clp-ffi-js.yaml`:
- Around line 6-10: The checksum filename variable G_CLP_FFI_JS_CHECKSUM_FILE
currently ends with .md5 but the pinned checksum is SHA‑256; update the value of
G_CLP_FFI_JS_CHECKSUM_FILE to use a .sha256 extension (e.g.,
"{{.G_BUILD_DIR}}/clp-ffi-js.sha256") and update any other occurrences/usages
(see the related checksum pin at lines referencing the SHA-256 value) so the
filename and the checksum algorithm match across G_CLP_FFI_JS_CHECKSUM_FILE and
the code that reads/verifies it.
- Around line 40-46: The inject-clp-src-dir step currently masks cmake failures
(the trailing "|| true") so a failed injection can go unnoticed and invalidate
tests; update the inject-clp-src-dir task to fail fast by removing "|| true"
and/or checking cmake's exit status after running the cmake command that sets
CLP_FFI_JS_CLP_SOURCE_DIRECTORY (the cmake invocation using G_CLP_FFI_JS_SRC_DIR
and G_CLP_FFI_JS_BUILD_DIR), and if cmake fails emit an error and exit non-zero
(or alternatively add an explicit verification that
CLP_FFI_JS_CLP_SOURCE_DIRECTORY was written to the cache/build metadata before
allowing the subsequent task test to run).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
taskfiles/tests/clp-ffi-js.yamltaskfiles/tests/main.yamltools/scripts/deps-download/init.shtools/yscope-dev-utils
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
taskfiles/tests/clp-ffi-js.yaml (1)
12-12:⚠️ Potential issue | 🟡 Minor
.md5extension vs SHA-256 checksum mismatch — already flagged.The filename ends in
.md5while the pinned checksum at line 78 is SHA-256.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfiles/tests/clp-ffi-js.yaml` at line 12, The checksum filename variable G_CLP_FFI_JS_CHECKSUM_FILE currently ends with “.md5” but the pinned checksum in this file is SHA-256; update the variable value to use a SHA-256-appropriate filename (e.g., change "{{.G_BUILD_DIR}}/clp-ffi-js.md5" to "{{.G_BUILD_DIR}}/clp-ffi-js.sha256" or similar) so the filename extension matches the actual checksum algorithm, or alternatively change the pinned checksum to an MD5 value if you intend to use MD5.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@taskfiles/tests/clp-ffi-js.yaml`:
- Line 79: The OUTPUT_DIR entry currently hardcodes
"{{.G_BUILD_DIR}}/clp-ffi-js"; change it to reuse the existing top-level
variable by setting OUTPUT_DIR to "{{.G_CLP_FFI_JS_SRC_DIR}}" so the path is
defined in one place and avoids duplication (update the OUTPUT_DIR key in the
taskfiles/tests/clp-ffi-js.yaml where OUTPUT_DIR is declared).
- Around line 36-37: The ln invocation uses an unquoted template variable
{{.ROOT_DIR}} which breaks on paths with spaces; update the command that creates
the symlink (the ln -s line referencing {{.ROOT_DIR}} and the target
build/deps/clp) to wrap the source path in quotes (consistent with the quoted
usage elsewhere) so the shell treats the entire path as a single argument.
- Around line 22-25: The developer workflow comments use the wrong task
namespace separator; replace the incorrect commands "task:clp-ffi-js",
"task:clp-ffi-js:build-and-test-node", and
"task:clp-ffi-js:build-and-test-browser" with the correct namespaced forms "task
tests:clp-ffi-js", "task tests:clp-ffi-js:build-and-test-node", and "task
tests:clp-ffi-js:build-and-test-browser" in the comments so readers run the
intended tasks (look for the comment block referencing clp-ffi-js and update
those command strings).
---
Duplicate comments:
In `@taskfiles/tests/clp-ffi-js.yaml`:
- Line 12: The checksum filename variable G_CLP_FFI_JS_CHECKSUM_FILE currently
ends with “.md5” but the pinned checksum in this file is SHA-256; update the
variable value to use a SHA-256-appropriate filename (e.g., change
"{{.G_BUILD_DIR}}/clp-ffi-js.md5" to "{{.G_BUILD_DIR}}/clp-ffi-js.sha256" or
similar) so the filename extension matches the actual checksum algorithm, or
alternatively change the pinned checksum to an MD5 value if you intend to use
MD5.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
taskfiles/tests/clp-ffi-js.yaml (4)
79-79:OUTPUT_DIRduplicates the path already captured byG_CLP_FFI_JS_SRC_DIR.Already noted in a previous review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfiles/tests/clp-ffi-js.yaml` at line 79, The OUTPUT_DIR entry duplicates the path already provided by G_CLP_FFI_JS_SRC_DIR; remove the redundant OUTPUT_DIR key or consolidate so the task uses G_CLP_FFI_JS_SRC_DIR as the single source of truth (update any task references that use OUTPUT_DIR to reference G_CLP_FFI_JS_SRC_DIR instead) to avoid duplication and keep the configuration consistent; ensure variables and dependent steps (those referencing OUTPUT_DIR) are adjusted to reference G_CLP_FFI_JS_SRC_DIR.
12-12: Checksum filename extension.md5is misleading for a SHA-256 digest.Already noted in a previous review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfiles/tests/clp-ffi-js.yaml` at line 12, The checksum filename currently uses a .md5 extension but the content is a SHA-256 digest; update the G_CLP_FFI_JS_CHECKSUM_FILE value to a SHA-256-appropriate filename (e.g., change "{{.G_BUILD_DIR}}/clp-ffi-js.md5" to "{{.G_BUILD_DIR}}/clp-ffi-js.sha256" or similar) and adjust any code or scripts that reference G_CLP_FFI_JS_CHECKSUM_FILE to expect the new extension so names and consumers remain consistent.
22-25: Developer-workflow comments use incorrect task invocation syntax.The commands in these comments should use a space separator (e.g.,
task tests:clp-ffi-js) rather thantask:clp-ffi-js. Already noted in a previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfiles/tests/clp-ffi-js.yaml` around lines 22 - 25, Update the developer workflow comments to use the correct task invocation syntax with a space separator: replace occurrences of "task:clp-ffi-js", "task:clp-ffi-js:build-and-test-node", and "task:clp-ffi-js:build-and-test-browser" with the space-separated form e.g. "task tests:clp-ffi-js", "task tests:clp-ffi-js build-and-test-node", and "task tests:clp-ffi-js build-and-test-browser" so the commands are accurate and consistent with the rest of the repo.
36-37: Unquoted{{.ROOT_DIR}}inln -scommand.Already noted in a previous review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfiles/tests/clp-ffi-js.yaml` around lines 36 - 37, The ln invocation uses an unquoted template expansion (the line starting with "ln -s {{.ROOT_DIR}} build/deps/clp"), which can break when ROOT_DIR contains spaces or glob characters; update that ln command to wrap the {{.ROOT_DIR}} expansion in double quotes so the symlink target is treated as a single literal argument (leave the preceding rm -rf build/deps/clp line unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@taskfiles/tests/clp-ffi-js.yaml`:
- Around line 49-51: Environment variable assignments export unquoted template
paths which will break on paths containing spaces; update the shell env
assignments that set VITE_NODE_MODULE_ABS_PATH and VITE_WORKER_MODULE_ABS_PATH
(and any similar template-made env vars like ROOT_DIR) to wrap the template
expansions in quotes so the full path is passed as a single value to npm/run
(e.g., change VITE_NODE_MODULE_ABS_PATH={{.G_CLP_FFI_JS_NODE_MODULE_PATH}} to
use quoted template expansion). Ensure all occurrences in the test runner blocks
are quoted consistently.
- Around line 26-39: Add a short inline comment in the default task near the sed
and subsequent task invocations to document why we expose downstream internal
tasks; specifically reference the downstream task names "node-modules" and
"config-cmake-project" and note that the sed command flips internal:true to
false in the pinned clp-ffi-js taskfile so those internal tasks can be invoked;
update the block around the sed line and the two task lines to include this
brief explanatory comment for future maintainers.
---
Duplicate comments:
In `@taskfiles/tests/clp-ffi-js.yaml`:
- Line 79: The OUTPUT_DIR entry duplicates the path already provided by
G_CLP_FFI_JS_SRC_DIR; remove the redundant OUTPUT_DIR key or consolidate so the
task uses G_CLP_FFI_JS_SRC_DIR as the single source of truth (update any task
references that use OUTPUT_DIR to reference G_CLP_FFI_JS_SRC_DIR instead) to
avoid duplication and keep the configuration consistent; ensure variables and
dependent steps (those referencing OUTPUT_DIR) are adjusted to reference
G_CLP_FFI_JS_SRC_DIR.
- Line 12: The checksum filename currently uses a .md5 extension but the content
is a SHA-256 digest; update the G_CLP_FFI_JS_CHECKSUM_FILE value to a
SHA-256-appropriate filename (e.g., change "{{.G_BUILD_DIR}}/clp-ffi-js.md5" to
"{{.G_BUILD_DIR}}/clp-ffi-js.sha256" or similar) and adjust any code or scripts
that reference G_CLP_FFI_JS_CHECKSUM_FILE to expect the new extension so names
and consumers remain consistent.
- Around line 22-25: Update the developer workflow comments to use the correct
task invocation syntax with a space separator: replace occurrences of
"task:clp-ffi-js", "task:clp-ffi-js:build-and-test-node", and
"task:clp-ffi-js:build-and-test-browser" with the space-separated form e.g.
"task tests:clp-ffi-js", "task tests:clp-ffi-js build-and-test-node", and "task
tests:clp-ffi-js build-and-test-browser" so the commands are accurate and
consistent with the rest of the repo.
- Around line 36-37: The ln invocation uses an unquoted template expansion (the
line starting with "ln -s {{.ROOT_DIR}} build/deps/clp"), which can break when
ROOT_DIR contains spaces or glob characters; update that ln command to wrap the
{{.ROOT_DIR}} expansion in double quotes so the symlink target is treated as a
single literal argument (leave the preceding rm -rf build/deps/clp line
unchanged).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
taskfiles/tests/clp-ffi-js.yaml (1)
12-12:⚠️ Potential issue | 🟡 MinorAlign the checksum filename extension with SHA‑256.
The checksum file name still ends with
.md5while the workflow pins a SHA‑256 digest, which is confusing in logs/tooling. Rename the file to reflect SHA‑256.♻️ Suggested tweak
- G_CLP_FFI_JS_CHECKSUM_FILE: "{{.G_BUILD_DIR}}/clp-ffi-js.md5" + G_CLP_FFI_JS_CHECKSUM_FILE: "{{.G_BUILD_DIR}}/clp-ffi-js.sha256"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfiles/tests/clp-ffi-js.yaml` at line 12, The checksum filename variable G_CLP_FFI_JS_CHECKSUM_FILE currently points to a file ending with ".md5" which is inconsistent with the workflow using a SHA-256 digest; update the value assigned to G_CLP_FFI_JS_CHECKSUM_FILE so the filename extension reflects SHA-256 (e.g., change "{{.G_BUILD_DIR}}/clp-ffi-js.md5" to "{{.G_BUILD_DIR}}/clp-ffi-js.sha256" or similar) and ensure any downstream references use the new name to avoid mismatches in logs and tooling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@taskfiles/tests/clp-ffi-js.yaml`:
- Line 12: The checksum filename variable G_CLP_FFI_JS_CHECKSUM_FILE currently
points to a file ending with ".md5" which is inconsistent with the workflow
using a SHA-256 digest; update the value assigned to G_CLP_FFI_JS_CHECKSUM_FILE
so the filename extension reflects SHA-256 (e.g., change
"{{.G_BUILD_DIR}}/clp-ffi-js.md5" to "{{.G_BUILD_DIR}}/clp-ffi-js.sha256" or
similar) and ensure any downstream references use the new name to avoid
mismatches in logs and tooling.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
taskfiles/tests/clp-ffi-js.yaml (2)
12-12:⚠️ Potential issue | 🟡 MinorChecksum file extension still mismatches the algorithm in use.
G_CLP_FFI_JS_CHECKSUM_FILEends in.md5whileFILE_SHA256on Line 84 uses SHA-256, creating a misleading filename.♻️ Proposed fix
- G_CLP_FFI_JS_CHECKSUM_FILE: "{{.G_BUILD_DIR}}/clp-ffi-js.md5" + G_CLP_FFI_JS_CHECKSUM_FILE: "{{.G_BUILD_DIR}}/clp-ffi-js.sha256"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfiles/tests/clp-ffi-js.yaml` at line 12, The checksum filename constant G_CLP_FFI_JS_CHECKSUM_FILE currently ends with .md5 but the pipeline uses FILE_SHA256 (SHA-256) — update G_CLP_FFI_JS_CHECKSUM_FILE to use a .sha256 extension (or otherwise match the SHA-256 naming convention) and ensure any places referencing G_CLP_FFI_JS_CHECKSUM_FILE or the produced checksum file are updated accordingly so the name and algorithm (see FILE_SHA256) are consistent.
33-36:⚠️ Potential issue | 🟡 MinorBlanket
sedover-exposes everyinternal: truetask in the downstream taskfile.The regex replaces all occurrences of
internal: trueintaskfile.yaml, not just the two tasks actually needed (node-modulesandconfig-cmake-project). Any other internal tasks in the pinned clp-ffi-js revision will silently become visible (e.g.,task --listoutput, accidental invocation).Consider scoping the replacement to only the two required tasks, or add a comment acknowledging the deliberate broad exposure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfiles/tests/clp-ffi-js.yaml` around lines 33 - 36, The current sed command blindly flips every "internal: true" to false in taskfile.yaml, exposing all internal tasks; change the replacement to target only the two tasks you need by scoping the sed operation to the specific task blocks (node-modules and config-cmake-project) or perform two targeted replacements (one for the node-modules block and one for the config-cmake-project block), or if the broad change is intentional, add an explicit comment above the sed step stating that exposing all internal tasks is deliberate; reference the task names "node-modules" and "config-cmake-project" and the existing sed invocation when implementing the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@taskfiles/tests/clp-ffi-js.yaml`:
- Line 12: The checksum filename constant G_CLP_FFI_JS_CHECKSUM_FILE currently
ends with .md5 but the pipeline uses FILE_SHA256 (SHA-256) — update
G_CLP_FFI_JS_CHECKSUM_FILE to use a .sha256 extension (or otherwise match the
SHA-256 naming convention) and ensure any places referencing
G_CLP_FFI_JS_CHECKSUM_FILE or the produced checksum file are updated accordingly
so the name and algorithm (see FILE_SHA256) are consistent.
- Around line 33-36: The current sed command blindly flips every "internal:
true" to false in taskfile.yaml, exposing all internal tasks; change the
replacement to target only the two tasks you need by scoping the sed operation
to the specific task blocks (node-modules and config-cmake-project) or perform
two targeted replacements (one for the node-modules block and one for the
config-cmake-project block), or if the broad change is intentional, add an
explicit comment above the sed step stating that exposing all internal tasks is
deliberate; reference the task names "node-modules" and "config-cmake-project"
and the existing sed invocation when implementing the fix.
hoophalab
left a comment
hoophalab
left a comment
There was a problem hiding this comment.
LGTM. One style comment.
This isn't actually executed within GitHub workflow, right? Might be best to change this PR title to end-to-end test task for downstream...
Validation: The task successfully downloads clp-ffi-js and executes the tests.
| deps: ["download-src"] | ||
| cmds: | ||
| # Expose `clp-ffi-js` tasks for test setup | ||
| - task: "symlink-yscope-dev-utils" |
There was a problem hiding this comment.
Why do we make symlink-yscope-dev-utils a separate sub-task while keep
rm -rf build/deps/clp
ln -s "{{.ROOT_DIR}}" build/deps/clp
inline? Might be cleaner to make both inline.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
the code lgtm
at a high level, do note that we are hardcoding the clp-ffi-js commit ref here, which means we may not be testing with the latest clp-ffi-js code. we probably shouldn't use the latest commit from clp-ffi-js anyways, like otherwise this flow that blocks PR submission cannot run deterministically. i wonder if it would be cleaner to instead set up some nightly flow in clp-ffi-js which builds clp-ffi-js against the latest version of CLP. if anything breaks, it will be clp-ffi-js' maintainers' (some of whom are clp's maintainers) onus to fix the issues / request fixes from the clp maintainers. what do you think?
| # Expose `clp-ffi-js` tasks for test setup | ||
| - task: "symlink-yscope-dev-utils" | ||
| - |- | ||
| sed -i 's/^\(\s*internal:\s*\)true\b/\1false/' taskfile.yaml |
There was a problem hiding this comment.
given the tasks in clp-ffi-js is now publicly (external to the clp-ffi-js) used, maybe we should change the tasks in clp-ffi-js to non-internal?
so long we dont document those tasks in clp-ffi-js docs, it shouldn't cause confusion to developers about what tasks they should run? if you really want, i think we can add inline docstrings to those tasks to explain thse are needed to set up the right dependencies for integration tests in the clp project. either way, it should be cleaner / more future-proof than this sed here
| FILE_SHA256: "5ab1c27031caafb014198d4db711a71b3fbb1661b396bcd4e46764194e2b1f61" | ||
| OUTPUT_DIR: "{{.G_CLP_FFI_JS_SRC_DIR}}" | ||
| TAR_FILE: "{{.G_BUILD_DIR}}/clp-ffi-js.tar.gz" | ||
| URL: "https://github.com/y-scope/clp-ffi-js/archive/4cc6a7c.tar.gz" |
There was a problem hiding this comment.
do you think it's worth adding a docstring to explain when this commit was available? / in the future if we pin those commit ids at specific clp-ffi-js releases, it's better to comment with the release versions as well
There was a problem hiding this comment.
something like
# clp-ffi-js @ v0.X.Y (March 2, 2026)
| npm run test -- --project node | ||
|
|
||
| # NOTE: Browser tests require installing browsers and system dependencies. The task is skipped if | ||
| # sudo is unavailable. |
There was a problem hiding this comment.
| # sudo is unavailable. | |
| # passwordless sudo is unavailable. |
| status: | ||
| - "! (command -v sudo >/dev/null && sudo -n true)" | ||
| cmds: | ||
| - |- |
There was a problem hiding this comment.
| status: | |
| - "! (command -v sudo >/dev/null && sudo -n true)" | |
| cmds: | |
| - |- | |
| cmds: | |
| - |- | |
| if ! (command -v sudo >/dev/null && sudo -n true 2>/dev/null); then | |
| echo "WARNING: Skipping browser tests (passwordless sudo is unavailable)." | |
| exit 0 | |
| fi |
| rm -rf build/deps/clp | ||
| ln -s "{{.ROOT_DIR}}" build/deps/clp |
There was a problem hiding this comment.
| rm -rf build/deps/clp | |
| ln -s "{{.ROOT_DIR}}" build/deps/clp | |
| rm -rf "build/deps/clp" | |
| ln -s "{{.ROOT_DIR}}" "build/deps/clp" |
There was a problem hiding this comment.
if there's no easy way to set up clp-ffi-js as one of deps for build-and-test-node and build-and-test-browser, how do you feel about adding this to both tasks:
preconditions:
- msg: "clp-ffi-js is not set up. Run `task tests:clp-ffi-js` first."
sh: "test -d {{.G_CLP_FFI_JS_BUILD_DIR}}"3 participants
Description
Since
clp-ffi-jsbuilds CLP from source using Emscripten and Clang as part of its WASM toolchain, this PR introduces end to end testing to ensure that changes in the CLP repository do not introduce build failures downstream inclp-ffi-js.Additionally, this PR bumps the
yscope-dev-utilsversion to incorporate the fix for the broken Boost argument handling described in y-scope/yscope-dev-utils#103Checklist
breaking change.
Validation performed
task tests:clp-ffi-jssucceeds locally.Summary by CodeRabbit
New Features
Tests
Chores