Add test suite, CI workflow, and technical documentation #7
Conversation
This commit introduces a comprehensive testing infrastructure, including: - Unit tests for parsers and integration tests for CLI extraction. - A GitHub Actions workflow for automated CI on Linux. - ARCHITECTURE.md and AGENTS.md for system design and agent guidance. - Build fixes for Linux (link flags and libgomp paths).
- Add --update-invariant to opam install for flambda compatibility - Add Ubuntu x86_64 path for libgomp.a in dune rule
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 QuickJS dune rules cd to %{project_root}/../../ causing build to run outside repo
The src/quickjs/dune rules cd to %{project_root}/../../ before running browserify, and also reference QuickJS tools/headers via %{project_root}/../../quickjs/....
%{project_root} is already the repository root, so appending /../../ moves the working directory two levels above the repo. That makes paths like src/quickjs/parsers.js and quickjs/qjsc resolve to non-existent locations.
Example (new code):
cd %{project_root}/../../
npx browserify src/quickjs/parsers.js > "$DIR/runtime.js"src/quickjs/parsers.js is expected to be in the repo root, but after cd it will not be.
Actual behavior: dune rules fail during build/test (cannot find src/quickjs/parsers.js / quickjs/qjsc / quickjs.h / libquickjs.a).
Expected behavior: these rules should run within the repo root (or otherwise reference repo-root-relative paths correctly).
Impact: CI and local builds that rely on these rules will break (QuickJS bridge can’t build, which typically blocks TypeScript/Pug parsing and may prevent the entire binary from linking).
(Refers to lines 24-50)
Recommendation: Remove the ../../ hops and operate from %{project_root} (repo root), e.g. cd %{project_root} and use %{project_root}/quickjs/... paths. Alternatively, avoid cd and pass absolute %{project_root}-based paths directly to browserify/qjsc/cp.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ROOT_SRC=\"$(cd ../../.. && pwd)\" | ||
| if [ -d \"$ROOT_SRC/strings\" ]; then | ||
| # Extraction generates 5 strings. | ||
| # We pre-populate 3 translations. | ||
| printf '\"Hello from HTML\" = \"Bonjour de HTML\";\n\"Hello from JS\" = \"Bonjour de JS\";\n\"Hello from Pug\" = \"Bonjour de Pug\";\n' > \"$ROOT_SRC/strings/french.strings\" | ||
| ./../src/cli/strings.exe \"$ROOT_SRC/tests/fixtures\" --output \"$ROOT_SRC/strings\" | ||
| fi |
There was a problem hiding this comment.
🔴 Integration test overwrites real strings/french.strings in repo root (data loss) when strings/ exists
The tests/dune runtest rule contains a “help user populate root strings/” block that writes directly into $ROOT_SRC/strings/french.strings and then runs the extractor with --output "$ROOT_SRC/strings".
New code:
ROOT_SRC="$(cd ../../.. && pwd)"
if [ -d "$ROOT_SRC/strings" ]; then
printf '...\n' > "$ROOT_SRC/strings/french.strings"
./../src/cli/strings.exe "$ROOT_SRC/tests/fixtures" --output "$ROOT_SRC/strings"
fiActual behavior: running dune runtest tests/ on a checkout that already has a strings/ directory will clobber the developer’s real strings/french.strings (and mutate other generated files) as a side effect of the test suite.
Expected behavior: tests should be hermetic and should not modify the user’s working tree or overwrite translation files.
Impact: potential data loss / corruption of translation files during routine test runs; also makes tests non-reproducible depending on whether strings/ exists.
Recommendation: Remove this side-effecting block, or gate it behind an explicit env var (e.g. POPULATE_ROOT_STRINGS=1), and never overwrite existing files (use a temp output dir or append-only behavior).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Note: I did this for my convenience to verify that the test command produced the correct output. @SGrondin If you prefer some other location feel free to modify as you see fit.
- Add &> /dev/null to the final fallback to suppress "No such file or directory" errors - Add touch libomp.a to ensure the target is always created even if all copies fail
- Replace fragile cp chain with a robust loop - Add Ubuntu/Debian /usr/lib/x86_64-linux-gnu/ paths - Remove touch hack to ensure real library is found - Added wildcard support for GCC versioned paths
| # Help user populate root strings/ if it exists | ||
| # We use absolute paths to ensure we hit the real source directory | ||
| # even if sandboxed in _build/default/tests | ||
| # The dune-project is used as a landmark for the root | ||
| # Traverse up to find the root of the source tree | ||
| # In dune, we are at _build/default/tests | ||
| ROOT_SRC=\"$(cd ../../.. && pwd)\" | ||
| if [ -d \"$ROOT_SRC/strings\" ]; then | ||
| # Extraction generates 5 strings. | ||
| # We pre-populate 3 translations. | ||
| printf '\"Hello from HTML\" = \"Bonjour de HTML\";\n\"Hello from JS\" = \"Bonjour de JS\";\n\"Hello from Pug\" = \"Bonjour de Pug\";\n' > \"$ROOT_SRC/strings/french.strings\" | ||
| ./../src/cli/strings.exe \"$ROOT_SRC/tests/fixtures\" --output \"$ROOT_SRC/strings\" | ||
| fi |
There was a problem hiding this comment.
🟡 Integration test helper computes repo root incorrectly (lands in _build instead of source root)
In tests/dune, the post-test helper tries to locate the repo root from the assumed working directory _build/default/tests:
# In dune, we are at _build/default/tests
ROOT_SRC="$(cd ../../.. && pwd)"From _build/default/tests, cd ../../.. resolves to _build, not the repository root. As a result, the conditional if [ -d "$ROOT_SRC/strings" ]; then ... almost always evaluates false (because _build/strings typically doesn’t exist), and the intended helper extraction into the real strings/ directory never runs.
Actual: helper block is effectively skipped even when the real repo-root strings/ exists.
Expected: compute the true repository root (likely one more .., or use %{project_root} from dune in the rule).
Impact: reduces usefulness of the test rule for developers; does not typically break CI because the helper is conditional.
Recommendation: Use dune variables instead of relative traversal, e.g. ROOT_SRC="%{project_root}" (or %{workspace_root} depending on dune version), or adjust the traversal to cd ../../../.. if you truly start from _build/default/tests.
Was this helpful? React with 👍 or 👎 to provide feedback.
Iterate over expanded glob patterns and select the first regular file to avoid "cp: target 'libomp.a' is not a directory" errors when multiple GCC versions are present.
Escape double quotes in the bash action to fix "Too many arguments for bash" dune parsing error.
This commit introduces a comprehensive testing infrastructure, including:
This PR was made with the help of Gemini 3 Flash Preview.