feat: add test init #96
Conversation
WalkthroughAdds a new CLI subcommand test-init, integrates it into the root command, refactors test command to pass arguments explicitly, introduces spec file generation logic and store, updates Specs struct to include a $schema field, and exports type accessors in analysis with corresponding test updates. New tests cover test-init behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant C as Cobra CLI
participant TI as test-init Command
participant P as Parser/Analyzer
participant I as Interpreter
participant FS as Filesystem
U->>C: numscript test-init <file.ns>
C->>TI: Execute Run(args)
TI->>FS: Read source file
TI->>P: Parse + analyze program
Note right of P: Collect variable declarations and types
P-->>TI: Program + inferred vars
TI->>I: Execute with TestInitStore
alt Missing funds
I-->>TI: Error: MissingFunds(account, asset, amount)
TI->>TI: Update TestInitStore balances
TI->>I: Retry execution
else Experimental feature
I-->>TI: Error: MissingFeatureFlag(name)
TI->>TI: Add feature flag
TI->>I: Retry execution
end
I-->>TI: Postings, balances
TI->>FS: Write <path>.specs.json ($schema, balances, vars, featureFlags, test)
TI-->>C: Success/err
C-->>U: Print result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
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 |
ascandone
force-pushed
the
feat/add-test-init
branch
from
54bdd17 to
bbbd48c
Compare
ascandone
force-pushed
the
feat/add-test-init
branch
from
bbbd48c to
17701c4
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #96 +/- ##
==========================================
- Coverage 70.89% 70.45% -0.45%
==========================================
Files 42 43 +1
Lines 4821 4982 +161
==========================================
+ Hits 3418 3510 +92
- Misses 1247 1313 +66
- Partials 156 159 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
internal/cmd/root.go(1 hunks)internal/cmd/test.go(2 hunks)internal/cmd/test_init.go(1 hunks)internal/cmd/test_init_test.go(1 hunks)internal/specs_format/index.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/cmd/test_init_test.go (1)
internal/cmd/test_init.go (1)
MakeSpecsFile(81-108)
internal/cmd/test_init.go (7)
internal/parser/ast.go (2)
VarDeclaration(308-313)Program(325-329)internal/analysis/union_find.go (2)
Type(9-11)TAsset(39-39)internal/analysis/check.go (7)
TypeMonetary(13-13)TypeNumber(17-17)TypeAccount(14-14)TypeString(18-18)TypePortion(15-15)TypeAsset(16-16)CheckProgram(290-294)internal/specs_format/index.go (2)
Specs(15-22)TestCase(24-46)internal/interpreter/interpreter.go (1)
RunProgram(228-285)internal/interpreter/interpreter_error.go (1)
MissingFundsErr(10-15)internal/utils/utils.go (2)
NestedMapGetOrPutDefault(60-66)MapGetOrPutDefault(50-58)
🪛 GitHub Actions: Default
internal/cmd/test_init.go
[error] 195-195: Error return value of os.WriteFile is not checked (errcheck)
| return makeSpecsFile( | ||
| program, | ||
| vars, | ||
| featureFlags, | ||
| &missingFundsErr.Needed, | ||
| ) |
There was a problem hiding this comment.
Fix default balance reuse for feature-flag retries.
...
...
But actual lines: in snippet `featureFlags[expFeatErr.FlagName]` etc around lines 145-152. Maybe use 145-152 to include entire block.
Need to mention we inadvertently reuse missingFundsErr pointer.
Should provide diff:
- defaultBalance,
No removal? we need diff.
Currently block:
return makeSpecsFile(
program,
vars,
featureFlags,
&missingFundsErr.Needed,
)
Change to:
return makeSpecsFile(
program,
vars,
featureFlags,
defaultBalance,
)
Yes.
Now comment for WriteFile ignoring error lines 193-197.
Better to include checking error.
We need to mention pipeline failure? but comment referencing watchers.
Use diff:
if err := os.WriteFile(...); err != nil { return err }
But they also print success after writing; we need to move Printf after success.
Better diff:
if err := os.WriteFile(...); err != nil {
return err
}
fmt.Printf...
makes sense.
Also we should mention still lighten.
Hence final comment.
No other major issues found.
Need to close file_end and final.
Let's craft.
Ensure we follow instructions: comment subject and short explanation.
For first comment: emphasise bug.
For second comment: mention pipeline failure.
Let's craft final response.
Need to ensure start and end markers.
Also no general summary? Not required but final response is the review (with file markers). Already abide.
Let's produce final answer.
<details>
<summary>🤖 Prompt for AI Agents</summary>
internal/cmd/test_init.go around lines 145-152 and 193-197: Fix two issues—(1)
when retrying with feature-flag errors we accidentally pass
&missingFundsErr.Needed (reusing the pointer) into makeSpecsFile; replace that
argument with the defaultBalance value so each retry uses the fresh default
balance instead of the previous error's pointer. (2) os.WriteFile errors are
ignored around lines 193-197; change the code to check the error returned by
os.WriteFile and return/handle it (and only print the success message after a
successful write) so file-write failures propagate and don’t leave the caller
unaware.
</details>
<!-- fingerprinting:phantom:medusa:sabertoothed -->
<!-- This is an auto-generated comment by CodeRabbit -->
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/analysis/check.go (1)
356-364: Guard against index out-of-range on built-in fn argsvalidArgs is indexed without length checks after reporting bad arity. With too few args, this will panic.
- FnVarOriginBalance/Overdraft need len(validArgs) >= 2
- FnVarOriginGetAsset needs len(validArgs) >= 1
Apply guards:
- case FnVarOriginBalance, FnVarOriginOverdraft: - // we run unify(<expr>, <asset>) in: - // <expr> := balance(@acc, <asset>) - res.unifyNodeWith(fnCall, res.GetExprType(validArgs[1])) + case FnVarOriginBalance, FnVarOriginOverdraft: + // we run unify(<expr>, <asset>) in: + // <expr> := balance(@acc, <asset>) + if len(validArgs) >= 2 { + res.unifyNodeWith(fnCall, res.GetExprType(validArgs[1])) + } - case FnVarOriginGetAsset: - res.unifyNodeWith(fnCall, res.GetExprType(validArgs[0])) + case FnVarOriginGetAsset: + if len(validArgs) >= 1 { + res.unifyNodeWith(fnCall, res.GetExprType(validArgs[0])) + }
🧹 Nitpick comments (4)
internal/cmd/test_init_test.go (3)
12-26: Preferrequire.NoErrorfor error assertions.The test logic is correct, but
require.NoError(t, err)is more idiomatic thanrequire.Nil(t, err)for error checks. It provides clearer intent and better error messages when the assertion fails.Apply this diff:
- require.Nil(t, err) + require.NoError(t, err)
28-35: Preferrequire.NoErrorfor error assertions.Same as the previous test, use
require.NoError(t, err)instead ofrequire.Nil(t, err)for clearer intent.Apply this diff:
- require.Nil(t, err) + require.NoError(t, err)
37-47: Preferrequire.NoErrorfor error assertions.Same as the previous tests, use
require.NoError(t, err)instead ofrequire.Nil(t, err)for clearer intent.Apply this diff:
- require.Nil(t, err) + require.NoError(t, err)internal/analysis/check.go (1)
443-444: Type unifications now use resolved accessorsUnifying monetary literal with its asset expr type, and infix LHS with RHS type, via GetExprType is consistent and clear. Good change.
Optional: add brief doc comments to GetExprType/GetVarDeclType clarifying “returns resolved type.”
Also applies to: 447-448
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
internal/analysis/check.go(6 hunks)internal/analysis/check_test.go(2 hunks)internal/cmd/test_init.go(1 hunks)internal/cmd/test_init_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cmd/test_init.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/analysis/check.go (2)
internal/parser/ast.go (4)
ValueExpr(8-11)VarDeclaration(308-313)BinaryInfix(20-20)BinaryInfix(69-74)internal/analysis/union_find.go (2)
Type(9-11)TVar(35-37)
internal/cmd/test_init_test.go (1)
internal/cmd/test_init.go (1)
MakeSpecsFile(81-108)
🔇 Additional comments (8)
internal/cmd/test_init_test.go (1)
1-10: LGTM!The package structure and imports are appropriate for testing the cmd package. The use of an external test package (
cmd_test) provides good separation.internal/analysis/check.go (5)
138-140: Good encapsulation: unexported mapsRenaming to private maps improves API hygiene and enforces accessor usage. Looks good.
163-165: Unifier now uses accessorUsing GetExprType in unifyNodeWith aligns with the new encapsulation. Good.
227-229: Init new mapsInitializing exprTypes/varTypes here is correct. No issues.
245-246: Var-decl unification via accessorSwitching to GetVarDeclType maintains consistency and avoids direct map access. LGTM.
142-150: Approve returning resolved types from accessors
No remaining direct.exprTypes/.varTypesusage or reliance on raw TVar pointer identity.internal/analysis/check_test.go (2)
1069-1071: Tests updated to accessorSwitch to res.GetVarDeclType is correct with the new API. LGTM.
1087-1088: Accessor usage in get_asset inference testCorrectly uses GetVarDeclType; assertions still validate Resolve(). Looks good.
2 participants
Generate a valid specs file which runs successfully against the given nusmcript, based on a few heuristics.
Script success is not guaranteed, but it should generate a valid file most of the time.