feat: improve testing tool #93
Conversation
WalkthroughAdds subset-compare utilities for maps/balances, updates specs framework to support focus/skip, new error expectations, end-balance assertions (including “include” semantics), records postings in results, and revises runner output and stats. Corresponding tests updated/added across interpreter, specs format, and runner. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant R as Runner
participant S as Specs Checker
participant I as Interpreter
participant U as Utils
R->>S: Load TestCases
S->>S: Detect focus across cases
loop For each TestCase
alt Skip or non-focused when any focused
S->>R: Record result {Skipped: true}
else Execute
S->>I: Run script with Vars/Balances
I-->>S: {Postings, Error?}
S->>S: If ExpectNegativeAmount / MissingFunds
alt Error expected
S->>S: Match specific error type
else No error expected
S->>S: Compute end balances
S->>U: CompareBalances or CompareBalancesIncluding
end
S-->>R: Record result {Pass/Fail, Postings}
end
end
R->>R: Aggregate {Passed, Failed, Skipped}
R-->>R: Print summary and per-test lines (on failures)
sequenceDiagram
autonumber
participant S as Specs Checker
participant U as Utils
participant B as Balances
S->>B: getBalances(postings)
B-->>S: endBalances
alt exact endBalances
S->>U: MapCmp (accounts) + MapCmp (assets)
else include endBalances.include
S->>U: MapIncludes (accounts)
U->>U: Nested MapIncludes (assets with equality)
end
U-->>S: bool matches
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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 |
ascandone
force-pushed
the
feat/improve-testing-tool
branch
from
abb2291 to
776e50b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #93 +/- ##
==========================================
- Coverage 70.56% 70.52% -0.05%
==========================================
Files 41 41
Lines 4631 4719 +88
==========================================
+ Hits 3268 3328 +60
- Misses 1212 1235 +23
- Partials 151 156 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ascandone
force-pushed
the
feat/improve-testing-tool
branch
from
201f00c to
dfb55a4
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/utils/utils.go (1)
74-87: Fix includes-argument order in MapIncludes to avoid false negativesThe predicate currently receives (subsetVal, supersetVal). This inverts semantics and will incorrectly fail when m1 has extra nested keys (e.g., superset account holds extra assets). Swap the arguments so the predicate is called with (supersetVal, subsetVal).
Apply this diff:
- for k1, v1 := range m2 { - v2, ok := m1[k1] - if !ok || !includes(v1, v2) { + for k1, v1 := range m2 { + v2, ok := m1[k1] + if !ok || !includes(v2, v1) { return false } }
🧹 Nitpick comments (8)
internal/utils/utils.go (1)
74-75: Clarify predicate semantics in the commentDocument that includes(xSuperset, xSubset) should return true when xSuperset “includes” xSubset. This prevents future call-site confusion.
internal/interpreter/balances_test.go (2)
83-145: Add a case where superset has extra assets for the same accountTo catch the MapIncludes argument-order bug and guard future regressions, add a subtest where b1["alice"] has an extra asset not present in b2["alice"]. Expected: true.
Add before the final closing brace:
+ t.Run("including with extra asset on same account (subset ok)", func(t *testing.T) { + b1 := Balances{ + "alice": AccountBalance{ + "EUR": big.NewInt(100), + "USD": big.NewInt(50), // extra asset in superset + }, + } + b2 := Balances{ + "alice": AccountBalance{ + "EUR": big.NewInt(100), + }, + } + require.True(t, CompareBalancesIncluding(b1, b2)) + })
101-101: Prefer require.True/False over comparing to booleansImproves readability and failure messages.
- require.Equal(t, true, CompareBalancesIncluding(b1, b2)) + require.True(t, CompareBalancesIncluding(b1, b2)) - require.Equal(t, false, CompareBalancesIncluding(b1, b2)) + require.False(t, CompareBalancesIncluding(b1, b2)) - require.Equal(t, false, CompareBalancesIncluding(b1, b2)) + require.False(t, CompareBalancesIncluding(b1, b2))Also applies to: 120-120, 143-143
internal/interpreter/balances.go (1)
104-111: Confirm MapIncludes predicate direction matches intended subset semanticsThis relies on MapIncludes calling includes(supersetVal, subsetVal). After fixing utils.MapIncludes as suggested, this function is correct. If you keep current utils semantics, invert here:
- return utils.MapIncludes(b1, b2, func(a1 AccountBalance, a2 AccountBalance) bool { - return utils.MapIncludes(a1, a2, func(i1 *big.Int, i2 *big.Int) bool { + return utils.MapIncludes(b1, b2, func(a2 AccountBalance, a1 AccountBalance) bool { + return utils.MapIncludes(a2, a1, func(i2 *big.Int, i1 *big.Int) bool { return i1.Cmp(i2) == 0 }) })After applying the utils fix, please run the new “extra asset” test to validate behavior.
internal/specs_format/index.go (4)
36-38: Expectation key naming mismatch with PR summary—verify contract.
Code usesexpect.error.missingFundsandexpect.error.negativeAmount. PR summary mentionedexpect.err.missingAmount. Ensure docs/spec files align to avoid breaking tests.I can generate a migration note or updater if you confirm the canonical keys.
48-57: Result shape additions (Skipped, Postings) are useful.
Minor: consideromitemptyonskippedandpostingsto reduce noise in JSON outputs when false/empty, unless consumers rely on explicit presence.- Skipped bool `json:"skipped"` + Skipped bool `json:"skipped,omitempty"` @@ - Postings []interpreter.Posting `json:"postings"` + Postings []interpreter.Posting `json:"postings,omitempty"`
172-183: Mismatch between JSON key and assertion label (“txMetadata” vs “txMeta”).
Failure messages useexpect.txMetabut JSON key isexpect.txMetadata. Align to avoid confusion.- failedAssertions = runAssertion[any](failedAssertions, - "expect.txMeta", + failedAssertions = runAssertion[any](failedAssertions, + "expect.txMetadata", testCase.ExpectTxMeta, metadata, reflect.DeepEqual, )
185-193: Mismatch between JSON key and assertion label (“metadata” vs “accountsMeta”).
JSON key isexpect.metadatabut label printsexpect.accountsMeta. Align for consistency.- failedAssertions = runAssertion[any](failedAssertions, - "expect.accountsMeta", + failedAssertions = runAssertion[any](failedAssertions, + "expect.metadata", testCase.ExpectAccountsMeta, result.AccountsMetadata, reflect.DeepEqual, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (13)
internal/interpreter/testdata/numscript-cookbook/experimental/mixed-source-prefer-single-source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/top-up.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/transfer-example.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/top-up-max.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restrict-balance-when-missing-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-all-failing.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/insufficient-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-not-enough-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-causes-failure.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source-allotment-invalid-amt.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/track-balances2.num.specs.jsonis excluded by!**/*.jsoninternal/specs_format/__snapshots__/runner_test.snapis excluded by!**/*.snap,!**/*.snapspecs.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (7)
internal/interpreter/balances.go(1 hunks)internal/interpreter/balances_test.go(1 hunks)internal/specs_format/index.go(8 hunks)internal/specs_format/run_test.go(6 hunks)internal/specs_format/runner.go(8 hunks)internal/specs_format/runner_test.go(4 hunks)internal/utils/utils.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
internal/interpreter/balances.go (2)
internal/interpreter/interpreter.go (2)
Balances(24-24)AccountBalance(23-23)internal/utils/utils.go (1)
MapIncludes(75-88)
internal/specs_format/runner_test.go (1)
internal/specs_format/runner.go (2)
RunSpecs(104-135)RawSpec(20-25)
internal/specs_format/index.go (3)
internal/interpreter/interpreter.go (4)
Balances(24-24)VariablesMap(15-15)AccountsMetadata(27-27)Posting(80-85)internal/interpreter/interpreter_error.go (2)
MissingFundsErr(10-15)NegativeAmountErr(125-128)internal/interpreter/balances.go (2)
CompareBalances(98-102)CompareBalancesIncluding(105-111)
internal/specs_format/run_test.go (2)
internal/interpreter/interpreter.go (5)
Posting(80-85)VariablesMap(15-15)Balances(24-24)AccountsMetadata(27-27)AccountBalance(23-23)internal/specs_format/index.go (4)
Specs(15-21)Check(84-249)SpecsResult(62-69)TestCaseResult(47-60)
internal/specs_format/runner.go (3)
internal/ansi/ansi.go (8)
ColorGreen(42-44)ColorBrightBlack(58-60)ColorRed(34-36)ColorYellow(46-48)Underline(93-95)Compose(10-17)ColorBrightYellow(70-72)Bold(89-91)internal/interpreter/interpreter.go (1)
PrettyPrintPostings(996-1003)internal/utils/utils.go (1)
Filter(40-48)
internal/interpreter/balances_test.go (2)
internal/interpreter/interpreter.go (2)
Balances(24-24)AccountBalance(23-23)internal/interpreter/balances.go (1)
CompareBalancesIncluding(105-111)
🔇 Additional comments (31)
internal/utils/utils.go (1)
68-72: Good simplification: MapCmp delegates to MapIncludesUsing cardinality + inclusion is clear and efficient for equality.
internal/specs_format/runner_test.go (5)
95-99: Rename to expect.endBalances looks goodUpdate aligns with end-state terminology.
104-104: Consistent error-key namespaceMoving to expect.error.* is clear and consistent.
111-111: Consistent error-key namespaceSame here; looks good.
197-198: Good rename to expect.error.missingFundsMatches the new convention.
215-258: Verify success flag expectation for focus scenarioWith one focused passing test and one skipped, confirm RunSpecs returns the intended boolean. If printFilesStats returns true when there are zero failures, this assertion may need to be True.
Run tests to confirm current contract; adjust the expectation if needed.
internal/specs_format/run_test.go (8)
67-74: Postings now part of results: LGTMThe explicit postings check strengthens the contract.
130-137: LGTM: postings captured in merged-outer caseValidates merge + execution path well.
151-152: LGTM: moved to expect.error.missingFundsNaming is coherent with other tests.
184-241: Assertion key updated: OKThe failure details now reference expect.error.missingFunds, consistent with the spec.
289-292: Explicit empty postings slice: goodEnsures JSON null vs empty slice semantics are tested.
296-340: New negative-amount expectation: solid coverageCovers the new error namespace and interpreter behavior.
343-373: Skip semantics test: LGTMStats and per-case flags are asserted clearly.
375-443: Focus semantics test: LGTMEnsures only focused tests run and others are skipped; postings are asserted.
internal/specs_format/runner.go (5)
137-139: Icons look good and consistent with ANSI palette.
Clear, readable, and matches the rest of the UI.
170-187: Nice unified header; consider stream consistency for fail summaries.
Headers print to stdout while detailed failures print to stderr (via showFailingTestCase). Mixing streams can interleave unexpectedly under pipes. If unintended, print both header and details to the same stream.Apply if you want headers on stderr as well:
- _, _ = fmt.Fprintf(stdout, "%s %s %s\n", ansi.ColorRed("❯"), rawSpec.NumscriptPath, testsCount) + _, _ = fmt.Fprintf(stderr, "%s %s %s\n", ansi.ColorRed("❯"), rawSpec.NumscriptPath, testsCount)Also applies to: 188-202
236-238: Good guard: skip non-failing/non-executed tests early.
Prevents noise in the failure output.
273-285: GOT section with postings is a solid DX improvement.
Shows postings or an explicit “”, which is helpful.
312-317: Stats partitioning is correct (pass/skip/fail).
Failed excludes skipped; good.Also applies to: 319-320
internal/specs_format/index.go (12)
7-8: Import of slices is appropriate for focus detection.
31-34: Focus/skip semantics: confirm precedence.
Current logic treats Skip as dominant even if Focus is true (a focused test with skip=true will be skipped). Verify this is intended.
39-45: End-balance assertions: include vs includes—verify the final JSON key.
You introducedexpect.endBalancesandexpect.endBalances.include. The PR summary mentioned “includes”. Confirm the intended spelling and update docs/tests consistently.
86-90: Focused test detection is clean and efficient.
92-101: Skip logic for (focus/skip) looks correct; counts are updated.
Good that skipped tests are recorded in SpecsResult and surfaced in the UI.
125-146: Error expectation handling is correct and type-specific.
Non-matching errors abort, which is reasonable.
194-201: End balances assertion uses initial balances + postings—correct.
Comparison viaCompareBalancesis appropriate.
203-210: Include semantics are correctly wired to CompareBalancesIncluding.
Name and comparator match the intent.
230-234: Safe postings capture when result is nil.
Prevents nil deref on error paths.
246-247: Total now includes skipped—good.
Invarianttotal == passing + failing + skippedholds.
301-316: getBalances is correct and side-effect free (clones initial).
Efficient and mirrors postings application.
31-45: Repo-wide verification for expect. keys — ripgrep skipped files; re-run checks*rg returned "No files were searched". Re-run one of these and paste the output:
# quick broad search (recommended) rg -n --hidden --no-ignore-vcs -g '!**/node_modules/**' -e 'expect\.' || true # or targeted keys rg -n --hidden --no-ignore-vcs -g '!**/node_modules/**' \ -e 'expect\.txMetadata' -e 'expect\.txMeta' -e 'expect\.metadata' -e 'expect\.accountsMeta' \ -e 'expect\.error\.missingFunds' -e 'expect\.error\.negativeAmount' \ -e 'expect\.endBalances' -e 'expect\.endBalances\.include' -e 'expect\.volumes' || trueAlso inspect internal/specs_format/index.go (lines 31-45, 172-183, 185-193) for the JSON tag names.
| skippedTestsCount := len(skippedTests) | ||
| passedTestsCount := len(passedTests) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t fail the run just because tests are skipped (especially with focus).
Currently, any skipped test makes RunSpecs return false, which will fail CI and also makes “focus” unusable without red status.
Consider success when there are no failures, irrespective of skips:
- return failedTestsCount+skippedTestsCount == 0
+ return failedTestsCount == 0Optionally, add a future flag to fail on skips if needed (e.g., CLI --fail-on-skip).
Also applies to: 379-383, 390-390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "locating candidate file..."
candidate=$(rg --hidden --files | rg 'internal/specs_format/runner.go$' || true)
if [ -z "$candidate" ]; then
candidate=$(rg -n --hidden 'func RunSpecs' -S | head -n1 | cut -d: -f1 || true)
fi
echo "candidate=$candidate"
if [ -z "$candidate" ] || [ ! -f "$candidate" ]; then
echo "file not found; listing matches for key identifiers:"
rg -n --hidden 'RunSpecs|skippedTestsCount|failedTestsCount' -S || true
exit 0
fi
file="$candidate"
echo "printing lines 360-400 from: $file"
nl -ba "$file" | sed -n '360,400p'
echo
echo "searching for occurrences of skippedTestsCount, failedTestsCount and return checks:"
rg -n -C3 'skippedTestsCount|failedTestsCount|return .*== 0' "$file" || trueLength of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="internal/specs_format/runner.go"
if [ ! -f "$file" ]; then
echo "file not found: $file"
rg -n --hidden 'RunSpecs|skippedTestsCount|failedTestsCount|skippedTests' -S || true
exit 0
fi
echo "printing lines 340-420 from: $file"
sed -n '340,420p' "$file" || true
echo
echo "searching for occurrences of skippedTestsCount, failedTestsCount, and return checks in the repo:"
rg -n -C3 --hidden 'skippedTestsCount|failedTestsCount|return .*== 0' -S || trueLength of output: 4707
Don't treat skipped tests as failures — return success when there are no failures.
internal/specs_format/runner.go currently returns failedTestsCount+skippedTestsCount == 0 (line 390). Change to failedTestsCount == 0.
- return failedTestsCount+skippedTestsCount == 0
+ return failedTestsCount == 0Optional: add a CLI flag (e.g., --fail-on-skip) to opt into failing on skips.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| skippedTestsCount := len(skippedTests) | |
| passedTestsCount := len(passedTests) | |
| skippedTestsCount := len(skippedTests) | |
| passedTestsCount := len(passedTests) | |
| return failedTestsCount == 0 |
🤖 Prompt for AI Agents
In internal/specs_format/runner.go around lines 365-367 (and update where the
final result is determined at ~line 390), the current exit/success check treats
skipped tests as failures; change the success condition to check only
failedTestsCount == 0 instead of failedTestsCount+skippedTestsCount == 0. Update
the return/value logic accordingly so skips do not make the run fail; optionally
add a CLI flag (e.g., --fail-on-skip) that, when set, changes the condition to
include skippedTestsCount in the failure check and wire that flag into the
result computation.
| "type": "string", | ||
| "description": "Test case description" | ||
| }, | ||
| "focus": { |
There was a problem hiding this comment.
If multiple tests have "focus" only the first one would be evaluated?
There was a problem hiding this comment.
No, if there's at least a test in focus mode, all and only tests in focus mode are run
Whereas skipped tests are never run
In any case, at least a skipped test will cause error exit code (so that we don't commit skipped tests by accident)
2 participants
A few improvements to the testing tool of the things I've found frustrating to work with.
expect.postings)expect.endVolumes.includesassertionexpect.volumesassertion name toexpect.endVolumesfor better consistencyexpect.missingAmountassertion name toexpect.err.missingAmountfor better consistency