Convert most shell tests into .clitest - #1234
Conversation
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
🤖 Augment PR SummarySummary: This PR migrates the majority of CLI integration tests from POSIX shell scripts to Sourcemeta Core’s Changes:
Technical Notes: Each DSL script runs in an isolated temporary directory; manifest assertions now use the interpreter’s deterministic 🤖 Was this summary useful? React with 👍 or 👎 |
There was a problem hiding this comment.
Benchmark Index (community)
Details
| Benchmark suite | Current: 57ae9e0 | Previous: 92c32af | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
260 ms |
324 ms |
0.80 |
Add one schema (100 existing) |
25 ms |
42 ms |
0.60 |
Add one schema (1000 existing) |
75 ms |
89 ms |
0.84 |
Add one schema (10000 existing) |
731 ms |
694 ms |
1.05 |
Update one schema (1 existing) |
20 ms |
29 ms |
0.69 |
Update one schema (101 existing) |
128 ms |
36 ms |
3.56 |
Update one schema (1001 existing) |
74 ms |
92 ms |
0.80 |
Update one schema (10001 existing) |
858 ms |
708 ms |
1.21 |
Cached rebuild (1 existing) |
6 ms |
9 ms |
0.67 |
Cached rebuild (101 existing) |
8 ms |
11 ms |
0.73 |
Cached rebuild (1001 existing) |
21 ms |
45 ms |
0.47 |
Cached rebuild (10001 existing) |
183 ms |
290 ms |
0.63 |
Index 100 schemas |
678 ms |
614 ms |
1.10 |
Index 1000 schemas |
1111 ms |
1453 ms |
0.76 |
Index 10000 schemas |
11165 ms |
12473 ms |
0.90 |
Index 10000 schemas (custom meta-schema) |
11692 ms |
14816 ms |
0.79 |
Index 10000 schemas ($ref fan-out) |
10905 ms |
15383 ms |
0.71 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Not reviewed (too large): test/cli/index/common/rebuild-zero-to-one.clitest (~2,392 lines), test/cli/index/common/rebuild-one-to-zero.clitest (~2,387 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/cli/index/common/fail-schema-non-string-ref.clitest">
<violation number="1" location="test/cli/index/common/fail-schema-non-string-ref.clitest:23">
P3: This new test is effectively a duplicate of `fail-invalid-schema-top-level-ref-draft7.clitest`. The two files differ only in the sibling schema (this one sets `$ref` to the integer `123` instead of `"#"` with a `type` sibling), yet both expect the exact same `SchemaReferenceObjectResourceError` output with the same path and identifier. Because that error is thrown solely because the schema has a top-level `$ref` in Draft 7 — independent of the `$ref` value's type — this test does not actually exercise non-string `$ref` handling distinctively; it would produce identical output whether `$ref` were a string or not. Consider consolidating the two tests or choosing a schema that triggers a truly distinct non-string `$ref` failure so the test's stated purpose (non-string `$ref`) is covered.</violation>
</file>
<file name="test/cli/index/common/rebuild-nested-directories.clitest">
<violation number="1" location="test/cli/index/common/rebuild-nested-directories.clitest:87">
P2: The expected output for run 2 contradicts the TODO comment next to it. The TODO states that adding s5 makes `dependents.metapack` rebuild for s1, s2, s3, and s4, but `expected.txt` only lists a single `Combining:` line (for s5's dependents) plus s5's Producing lines. Because the `DROP LINES` filter keeps anything containing "Producing|Combining", any extra dependents lines for s1–s4 would survive the filter and make the `COMPARE` fail; conversely, if they do not appear, the TODO's description of the over-rebuild does not match the asserted output. Confirm which is true so the test and comment agree.</violation>
</file>
<file name="test/cli/index/common/fail-no-arguments.clitest">
<violation number="1" location="test/cli/index/common/fail-no-arguments.clitest:1">
P3: This whole test duplicates the final RUN case already present in test/cli/index/common/output-help-skip-banner.clitest. That file's last block runs `RUN --skip-banner STDIN /dev/null IN . INTO output.txt EXPECTING 1` and compares against the identical usage output, so the no-arguments failure path is already covered. Since these files are separate, this adds a second independent copy of the expected usage text that must be kept in sync whenever the help output changes. Drop this file (and register no `sourcemeta_one_test_cli` entry for it) or, if a dedicated test is wanted, remove the duplicated third block from output-help-skip-banner.clitest instead.</violation>
</file>
<file name="test/cli/index/enterprise/rebuild-removed-global-artifact.clitest">
<violation number="1" location="test/cli/index/enterprise/rebuild-removed-global-artifact.clitest:109">
P2: The file's own documentation and its assertions cannot both hold. The header comment states that a removed global artifact is "never produced again" (the rebuild trusts state and does not regenerate it), and the test removes `output/authentication.bin` and `output/routes.bin` to exercise this. Yet `expected.txt`, written once and compared against the post-rebuild manifest, still lists `./authentication.bin` and `./routes.bin`. If the artifact is truly never reproduced, the post-rebuild manifest omits those entries and `COMPARE manifest.txt AGAINST expected.txt` fails; if it is reproduced, the test passes but does not test the bug the comment describes (and the comment is false). Either the expected manifest must reflect the post-removal output (dropping the removed artifacts) so the test locks in the documented behavior, or the comment misdescribes what the rebuild does. Please reconcile the documented behavior with the expected output.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| COPY output.txt TO output_producing.txt | ||
| DROP LINES MATCHING '^(?!.*(Producing|Combining)).*$' IN output_producing.txt | ||
|
|
||
| // TODO(over-rebuild): Adding s5 causes dependents.metapack to rebuild for |
There was a problem hiding this comment.
P2: The expected output for run 2 contradicts the TODO comment next to it. The TODO states that adding s5 makes dependents.metapack rebuild for s1, s2, s3, and s4, but expected.txt only lists a single Combining: line (for s5's dependents) plus s5's Producing lines. Because the DROP LINES filter keeps anything containing "Producing|Combining", any extra dependents lines for s1–s4 would survive the filter and make the COMPARE fail; conversely, if they do not appear, the TODO's description of the over-rebuild does not match the asserted output. Confirm which is true so the test and comment agree.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/cli/index/common/rebuild-nested-directories.clitest, line 87:
<comment>The expected output for run 2 contradicts the TODO comment next to it. The TODO states that adding s5 makes `dependents.metapack` rebuild for s1, s2, s3, and s4, but `expected.txt` only lists a single `Combining:` line (for s5's dependents) plus s5's Producing lines. Because the `DROP LINES` filter keeps anything containing "Producing|Combining", any extra dependents lines for s1–s4 would survive the filter and make the `COMPARE` fail; conversely, if they do not appear, the TODO's description of the over-rebuild does not match the asserted output. Confirm which is true so the test and comment agree.</comment>
<file context>
@@ -0,0 +1,115 @@
+COPY output.txt TO output_producing.txt
+DROP LINES MATCHING '^(?!.*(Producing|Combining)).*$' IN output_producing.txt
+
+// TODO(over-rebuild): Adding s5 causes dependents.metapack to rebuild for
+// s1, s2, s3, and s4 even though none of these schemas reference each other.
+// The dependency-tree.metapack global aggregate triggers ForceOnGraphChange
</file context>
| DROP LINES MATCHING '^\./explorer/public/self(/.*)?$' IN manifest.txt | ||
| DROP LINES MATCHING '^\./explorer/alpha/self(/.*)?$' IN manifest.txt | ||
|
|
||
| COMPARE manifest.txt AGAINST expected.txt |
There was a problem hiding this comment.
P2: The file's own documentation and its assertions cannot both hold. The header comment states that a removed global artifact is "never produced again" (the rebuild trusts state and does not regenerate it), and the test removes output/authentication.bin and output/routes.bin to exercise this. Yet expected.txt, written once and compared against the post-rebuild manifest, still lists ./authentication.bin and ./routes.bin. If the artifact is truly never reproduced, the post-rebuild manifest omits those entries and COMPARE manifest.txt AGAINST expected.txt fails; if it is reproduced, the test passes but does not test the bug the comment describes (and the comment is false). Either the expected manifest must reflect the post-removal output (dropping the removed artifacts) so the test locks in the documented behavior, or the comment misdescribes what the rebuild does. Please reconcile the documented behavior with the expected output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/cli/index/enterprise/rebuild-removed-global-artifact.clitest, line 109:
<comment>The file's own documentation and its assertions cannot both hold. The header comment states that a removed global artifact is "never produced again" (the rebuild trusts state and does not regenerate it), and the test removes `output/authentication.bin` and `output/routes.bin` to exercise this. Yet `expected.txt`, written once and compared against the post-rebuild manifest, still lists `./authentication.bin` and `./routes.bin`. If the artifact is truly never reproduced, the post-rebuild manifest omits those entries and `COMPARE manifest.txt AGAINST expected.txt` fails; if it is reproduced, the test passes but does not test the bug the comment describes (and the comment is false). Either the expected manifest must reflect the post-removal output (dropping the removed artifacts) so the test locks in the documented behavior, or the comment misdescribes what the rebuild does. Please reconcile the documented behavior with the expected output.</comment>
<file context>
@@ -0,0 +1,123 @@
+DROP LINES MATCHING '^\./explorer/public/self(/.*)?$' IN manifest.txt
+DROP LINES MATCHING '^\./explorer/alpha/self(/.*)?$' IN manifest.txt
+
+COMPARE manifest.txt AGAINST expected.txt
+
+REMOVE output/routes.bin
</file context>
| { | ||
| "$schema": "http://json-schema.org/draft-07/schema#", | ||
| "$id": "https://example.com/test", | ||
| "$ref": 123 |
There was a problem hiding this comment.
P3: This new test is effectively a duplicate of fail-invalid-schema-top-level-ref-draft7.clitest. The two files differ only in the sibling schema (this one sets $ref to the integer 123 instead of "#" with a type sibling), yet both expect the exact same SchemaReferenceObjectResourceError output with the same path and identifier. Because that error is thrown solely because the schema has a top-level $ref in Draft 7 — independent of the $ref value's type — this test does not actually exercise non-string $ref handling distinctively; it would produce identical output whether $ref were a string or not. Consider consolidating the two tests or choosing a schema that triggers a truly distinct non-string $ref failure so the test's stated purpose (non-string $ref) is covered.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/cli/index/common/fail-schema-non-string-ref.clitest, line 23:
<comment>This new test is effectively a duplicate of `fail-invalid-schema-top-level-ref-draft7.clitest`. The two files differ only in the sibling schema (this one sets `$ref` to the integer `123` instead of `"#"` with a `type` sibling), yet both expect the exact same `SchemaReferenceObjectResourceError` output with the same path and identifier. Because that error is thrown solely because the schema has a top-level `$ref` in Draft 7 — independent of the `$ref` value's type — this test does not actually exercise non-string `$ref` handling distinctively; it would produce identical output whether `$ref` were a string or not. Consider consolidating the two tests or choosing a schema that triggers a truly distinct non-string `$ref` failure so the test's stated purpose (non-string `$ref`) is covered.</comment>
<file context>
@@ -0,0 +1,37 @@
+{
+ "$schema": "http://json-schema.org/draft-07/schema#",
+ "$id": "https://example.com/test",
+ "$ref": 123
+}
+EOF
</file context>
There was a problem hiding this comment.
Benchmark Index (enterprise)
Details
| Benchmark suite | Current: 57ae9e0 | Previous: 92c32af | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
348 ms |
372 ms |
0.94 |
Add one schema (100 existing) |
123 ms |
131 ms |
0.94 |
Add one schema (1000 existing) |
168 ms |
174 ms |
0.97 |
Add one schema (10000 existing) |
861 ms |
785 ms |
1.10 |
Update one schema (1 existing) |
98 ms |
111 ms |
0.88 |
Update one schema (101 existing) |
112 ms |
126 ms |
0.89 |
Update one schema (1001 existing) |
176 ms |
176 ms |
1 |
Update one schema (10001 existing) |
877 ms |
785 ms |
1.12 |
Cached rebuild (1 existing) |
10 ms |
10 ms |
1 |
Cached rebuild (101 existing) |
13 ms |
14 ms |
0.93 |
Cached rebuild (1001 existing) |
28 ms |
51 ms |
0.55 |
Cached rebuild (10001 existing) |
204 ms |
336 ms |
0.61 |
Index 100 schemas |
679 ms |
463 ms |
1.47 |
Index 1000 schemas |
1281 ms |
1289 ms |
0.99 |
Index 10000 schemas |
13512 ms |
13043 ms |
1.04 |
Index 10000 schemas (custom meta-schema) |
16087 ms |
14823 ms |
1.09 |
Index 10000 schemas ($ref fan-out) |
15622 ms |
15086 ms |
1.04 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
1 issue found across 79 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/cli/index/common/fail-flag-unexpected-value.clitest">
<violation number="1" location="test/cli/index/common/fail-flag-unexpected-value.clitest:2">
P2: This delta deletes the `DROP LINES MATCHING '^2>' IN output.txt` normalization from `fail-flag-unexpected-value.clitest`, while every sibling index test (`fail-invalid-schema.clitest`, `fail-schema-boolean.clitest`, `fail-unknown-dialect.clitest`, etc.) keeps it. With the drop removed, `COMPARE output.txt AGAINST expected.txt` will fail the whole test if the `sourcemeta-one index` binary emits any stderr (`2>`-prefixed) lines on the `--verbose=true` option-error path, because `expected.txt` contains only `1>` lines. If the removal is intentional (the option parser errors before any verbose output is produced), the other tests' drops are themselves unnecessary; otherwise the test is now fragile. Confirm there is genuinely no stderr output on this path, or restore the drop for consistency and robustness.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -0,0 +1,8 @@ | |||
| RUN --verbose=true STDIN /dev/null IN . INTO output.txt EXPECTING 1 | |||
There was a problem hiding this comment.
P2: This delta deletes the DROP LINES MATCHING '^2>' IN output.txt normalization from fail-flag-unexpected-value.clitest, while every sibling index test (fail-invalid-schema.clitest, fail-schema-boolean.clitest, fail-unknown-dialect.clitest, etc.) keeps it. With the drop removed, COMPARE output.txt AGAINST expected.txt will fail the whole test if the sourcemeta-one index binary emits any stderr (2>-prefixed) lines on the --verbose=true option-error path, because expected.txt contains only 1> lines. If the removal is intentional (the option parser errors before any verbose output is produced), the other tests' drops are themselves unnecessary; otherwise the test is now fragile. Confirm there is genuinely no stderr output on this path, or restore the drop for consistency and robustness.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/cli/index/common/fail-flag-unexpected-value.clitest, line 2:
<comment>This delta deletes the `DROP LINES MATCHING '^2>' IN output.txt` normalization from `fail-flag-unexpected-value.clitest`, while every sibling index test (`fail-invalid-schema.clitest`, `fail-schema-boolean.clitest`, `fail-unknown-dialect.clitest`, etc.) keeps it. With the drop removed, `COMPARE output.txt AGAINST expected.txt` will fail the whole test if the `sourcemeta-one index` binary emits any stderr (`2>`-prefixed) lines on the `--verbose=true` option-error path, because `expected.txt` contains only `1>` lines. If the removal is intentional (the option parser errors before any verbose output is produced), the other tests' drops are themselves unnecessary; otherwise the test is now fragile. Confirm there is genuinely no stderr output on this path, or restore the drop for consistency and robustness.</comment>
<file context>
@@ -1,5 +1,4 @@
RUN --verbose=true STDIN /dev/null IN . INTO output.txt EXPECTING 1
-DROP LINES MATCHING '^2>' IN output.txt
WRITE expected.txt UNTIL EOF
1> error: This flag cannot take a value
</file context>
There was a problem hiding this comment.
All reported issues were addressed across 70 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 69 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/cli/index/common/snapshot-no-self-version-filename.clitest">
<violation number="1" location="test/cli/index/common/snapshot-no-self-version-filename.clitest:99">
P3: This change pins the exact enumeration of all 64 embedded self schemas and the exact version/edition banner into this test's expected log. Any unrelated change to the self schema set (add, remove, or rename a file under src/) or to the banner format will break this test, even though the test's purpose is only the no-self-version-filename manifest check. The prior shell test ran the command and asserted only the output tree, avoiding this coupling. Consider reducing the log assertion to the specific lines this test cares about rather than the full schema listing.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| 2> Detecting: [PREFIX]/share/sourcemeta/one/self/v1/schemas/api/schemas/output-error.json (#63) | ||
| 2> Detecting: [PREFIX]/share/sourcemeta/one/self/v1/schemas/api/list/response.json (#64) | ||
| EOF | ||
| COMPARE log_1.txt AGAINST expected_log_1.txt |
There was a problem hiding this comment.
P3: This change pins the exact enumeration of all 64 embedded self schemas and the exact version/edition banner into this test's expected log. Any unrelated change to the self schema set (add, remove, or rename a file under src/) or to the banner format will break this test, even though the test's purpose is only the no-self-version-filename manifest check. The prior shell test ran the command and asserted only the output tree, avoiding this coupling. Consider reducing the log assertion to the specific lines this test cares about rather than the full schema listing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/cli/index/common/snapshot-no-self-version-filename.clitest, line 99:
<comment>This change pins the exact enumeration of all 64 embedded self schemas and the exact version/edition banner into this test's expected log. Any unrelated change to the self schema set (add, remove, or rename a file under src/) or to the banner format will break this test, even though the test's purpose is only the no-self-version-filename manifest check. The prior shell test ran the command and asserted only the output tree, avoiding this coupling. Consider reducing the log assertion to the specific lines this test cares about rather than the full schema listing.</comment>
<file context>
@@ -20,15 +20,83 @@ WRITE one.json UNTIL EOF
+2> Detecting: [PREFIX]/share/sourcemeta/one/self/v1/schemas/api/list/response.json (#64)
EOF
-COMPARE unchecked_1.txt AGAINST nothing.txt
+COMPARE log_1.txt AGAINST expected_log_1.txt
TREE output INTO manifest.txt
</file context>
There was a problem hiding this comment.
All reported issues were addressed across 67 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Signed-off-by: Juan Cruz Viotti jv@jviotti.com