feat: specs format #70
Conversation
WalkthroughThe changes introduce a comprehensive test runner for Numscript programs, including new CLI commands, test specification parsing, assertion checking, and pretty-printing utilities. The ANSI styling and CSV formatting capabilities are expanded, and several internal APIs are refactored for clarity and consistency. Extensive tests and snapshot-based validation are added for new features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant TestCmd
participant SpecsRunner
participant Interpreter
participant Reporter
User->>CLI: Run `test` command
CLI->>TestCmd: Parse args, invoke getTestCmd()
TestCmd->>SpecsRunner: readSpecsFiles()
SpecsRunner->>SpecsRunner: Discover *.num.specs.json files
SpecsRunner->>SpecsRunner: Load Numscript and specs JSON
SpecsRunner->>Interpreter: Parse Numscript
SpecsRunner->>SpecsRunner: Unmarshal specs JSON
SpecsRunner->>Interpreter: Run Check() for each test case
Interpreter-->>SpecsRunner: Return results per test
SpecsRunner->>Reporter: Print failures, diffs, summary
Reporter-->>User: Display test results
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #70 +/- ##
==========================================
- Coverage 72.28% 71.10% -1.18%
==========================================
Files 37 41 +4
Lines 4091 4676 +585
==========================================
+ Hits 2957 3325 +368
- Misses 1006 1211 +205
- Partials 128 140 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ascandone
force-pushed
the
feat/specs-format
branch
from
2293b4f to
5d93aca
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (16)
internal/specs_format/parse_test.go (2)
45-45: Userequire.NoErrorinstead ofrequire.Nilfor error assertions.This is the idiomatic way to assert no error occurred and provides better error messages.
- require.Nil(t, err) + require.NoError(t, err)
13-76: Consider adding edge case tests for robustness.The current test only covers a single happy path scenario. Consider adding tests for:
- Empty JSON objects
- Invalid JSON format
- Missing required fields
- Invalid field types
- Large numeric values
This would help ensure the JSON unmarshaling handles various input scenarios correctly.
internal/utils/pretty_csv_test.go (1)
10-30: Consider adding edge case tests for better coverage.The current tests only cover basic functionality. Consider adding tests for:
- Empty data (no rows)
- Mismatched column counts
- Very long strings that might affect formatting
- Special characters in data
- Large datasets
This would help ensure the pretty printing functions handle various edge cases gracefully.
internal/interpreter/interpreter.go (2)
996-1003: Consider adding input validation for robustness.The
PrettyPrintPostingsfunction doesn't validate the input slice. Consider adding a nil check or handling empty slices explicitly.func PrettyPrintPostings(postings []Posting) string { + if len(postings) == 0 { + return utils.CsvPretty([]string{"Source", "Destination", "Asset", "Amount"}, [][]string{}, false) + } var rows [][]string for _, posting := range postings { row := []string{posting.Source, posting.Destination, posting.Asset, posting.Amount.String()} rows = append(rows, row) } return utils.CsvPretty([]string{"Source", "Destination", "Asset", "Amount"}, rows, false) }
1005-1012: Consider adding input validation for robustness.The
PrettyPrintMetafunction doesn't validate the input map. Consider adding a nil check.func PrettyPrintMeta(meta Metadata) string { + if meta == nil { + return utils.CsvPrettyMap("Name", "Value", map[string]string{}) + } m := map[string]string{} for k, v := range meta { m[k] = v.String() } return utils.CsvPrettyMap("Name", "Value", m) }internal/interpreter/balances.go (2)
85-96: Consider adding input validation for robustness.The
PrettyPrintfunction doesn't validate the receiver. Consider adding a nil check or handling empty balances explicitly.func (b Balances) PrettyPrint() string { + if b == nil || len(b) == 0 { + return utils.CsvPretty([]string{"Account", "Asset", "Balance"}, [][]string{}, true) + } header := []string{"Account", "Asset", "Balance"} var rows [][]string for account, accBalances := range b { for asset, balance := range accBalances { row := []string{account, asset, balance.String()} rows = append(rows, row) } } return utils.CsvPretty(header, rows, true) }
98-102: Consider adding input validation for robustness.The
CompareBalancesfunction doesn't validate its inputs. Consider adding nil checks for safety.func CompareBalances(b1 Balances, b2 Balances) bool { + if b1 == nil && b2 == nil { + return true + } + if b1 == nil || b2 == nil { + return false + } return utils.Map2Cmp(b1, b2, func(ab1, ab2 *big.Int) bool { return ab1.Cmp(ab2) == 0 }) }internal/utils/pretty_csv.go (2)
76-83: Consider adding input validation for robustness.The
CsvPrettyMapfunction doesn't validate its inputs. Consider adding a nil check for the map parameter.func CsvPrettyMap(keyName string, valueName string, m map[string]string) string { + if m == nil { + return CsvPretty([]string{keyName, valueName}, [][]string{}, true) + } var rows [][]string for k, v := range m { rows = append(rows, []string{k, v}) } return CsvPretty([]string{keyName, valueName}, rows, true) }
17-29: Consider optimizing the sorting logic for better performance.The current sorting concatenates strings with
|separator, which could be inefficient for large datasets. Consider using a more efficient comparison approach.if sortRows { slices.SortStableFunc(rows, func(x, y []string) int { - strX := strings.Join(x, "|") - strY := strings.Join(y, "|") - if strX == strY { - return 0 - } else if strX < strY { - return -1 - } else { - return 1 - } + // Compare column by column for better performance + for i := 0; i < len(x) && i < len(y); i++ { + if cmp := strings.Compare(x[i], y[i]); cmp != 0 { + return cmp + } + } + return len(x) - len(y) }) }internal/interpreter/accounts_metadata.go (1)
26-36: Improve variable naming for clarityThe variable names are confusing and don't match the actual data types being handled.
Apply this diff to improve variable naming:
func (m AccountsMetadata) Merge(update AccountsMetadata) { - for acc, accBalances := range update { + for acc, accMetadata := range update { cachedAcc := utils.MapGetOrPutDefault(m, acc, func() AccountMetadata { return AccountMetadata{} }) - for curr, amt := range accBalances { - cachedAcc[curr] = amt + for metadataKey, metadataValue := range accMetadata { + cachedAcc[metadataKey] = metadataValue } } }internal/cmd/test.go (3)
48-53: Address the TODO comment for better error messaging.The TODO indicates that error messages should be improved when no matching numscript file is found for a specs file.
Would you like me to implement improved error messaging that clearly indicates when a specs file exists but its corresponding numscript file is missing?
269-269: Remove misleading comment.The comment suggests hardcoding, but the implementation actually calculates maxLen dynamically.
- maxLen := max(len(testFilesLabel), len(testsLabel)) // yeah, ok, this could be hardcoded, I know + maxLen := max(len(testFilesLabel), len(testsLabel))
276-282: Consider using a map for more efficient unique file counting.Using
CompactFuncrequires sorting and is less efficient than using a map to track unique files.- filesCount := len(slices.CompactFunc(allTests, func(t1 testResult, t2 testResult) bool { - return t1.File == t2.File - })) - failedTestsFilesCount := len(slices.CompactFunc(failedTests, func(t1 testResult, t2 testResult) bool { - return t1.File == t2.File - })) + uniqueFiles := make(map[string]bool) + uniqueFailedFiles := make(map[string]bool) + for _, test := range allTests { + uniqueFiles[test.File] = true + } + for _, test := range failedTests { + uniqueFailedFiles[test.File] = true + } + filesCount := len(uniqueFiles) + failedTestsFilesCount := len(uniqueFailedFiles)internal/specs_format/index.go (3)
98-101: Improve error context for type assertion.The type assertion doesn't provide context about what type of error was actually encountered.
_, ok := err.(interpreter.MissingFundsErr) if !ok { - return SpecsResult{}, err + return SpecsResult{}, fmt.Errorf("unexpected error type during test execution: %w", err) }Add the fmt import at the top of the file:
+ "fmt"
221-240: Add tests for the Movements type and getMovements function.The TODO comment indicates that tests are missing for this functionality.
Would you like me to generate unit tests for the
Movementstype andgetMovementsfunction to ensure correct aggregation of posting movements?
242-257: Consider adding a comment to clarify that volumes represent final balances.The function correctly calculates final balances after applying postings, but the term "volumes" might be confusing.
+// getVolumes calculates the final account balances after applying all postings to the initial balances func getVolumes(postings []interpreter.Posting, initialBalances interpreter.Balances) interpreter.Balances {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.suminternal/cmd/__snapshots__/test_test.snapis excluded by!**/*.snap,!**/*.snapinternal/interpreter/__snapshots__/balances_test.snapis excluded by!**/*.snap,!**/*.snapinternal/utils/__snapshots__/pretty_csv_test.snapis excluded by!**/*.snap,!**/*.snapspecs.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (20)
.gitignore(1 hunks)internal/ansi/ansi.go(1 hunks)internal/cmd/root.go(1 hunks)internal/cmd/run.go(6 hunks)internal/cmd/test.go(1 hunks)internal/cmd/test_test.go(1 hunks)internal/interpreter/accounts_metadata.go(1 hunks)internal/interpreter/balances.go(4 hunks)internal/interpreter/balances_test.go(2 hunks)internal/interpreter/batch_balances_query.go(1 hunks)internal/interpreter/function_statements.go(2 hunks)internal/interpreter/interpreter.go(3 hunks)internal/interpreter/utils.go(0 hunks)internal/parser/parser.go(2 hunks)internal/specs_format/index.go(1 hunks)internal/specs_format/parse_test.go(1 hunks)internal/specs_format/run_test.go(1 hunks)internal/utils/pretty_csv.go(1 hunks)internal/utils/pretty_csv_test.go(1 hunks)internal/utils/utils.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/interpreter/utils.go
🧰 Additional context used
🧠 Learnings (3)
internal/specs_format/run_test.go (4)
Learnt from: ascandone
PR: formancehq/numscript#45
File: internal/parser/ast.go:21-21
Timestamp: 2025-03-18T11:14:21.411Z
Learning: In Go, empty methods like `valueExpr()` are commonly used as interface markers and don't require dedicated unit tests as they have no functionality to test. The actual behavior is tested through integration tests that use the types in context.
Learnt from: ascandone
PR: formancehq/numscript#27
File: internal/interpreter/interpreter.go:667-668
Timestamp: 2024-12-05T11:42:58.472Z
Learning: In Go test files within the `internal/interpreter` package (e.g., `reconciler_test.go` and `interpreter_test.go`), it's acceptable to use hardcoded `"<kept>"` strings in test data and comments, and they do not need to be replaced with the `KEPT_ADDR` constant.
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.961Z
Learning: Edge cases for oneof destination parsing in numscript are covered by the existing inorder parsing fault tolerance tests, as both constructs share similar parsing logic.
internal/cmd/test_test.go (3)
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
Learnt from: ascandone
PR: formancehq/numscript#45
File: internal/parser/ast.go:21-21
Timestamp: 2025-03-18T11:14:21.411Z
Learning: In Go, empty methods like `valueExpr()` are commonly used as interface markers and don't require dedicated unit tests as they have no functionality to test. The actual behavior is tested through integration tests that use the types in context.
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.961Z
Learning: Edge cases for oneof destination parsing in numscript are covered by the existing inorder parsing fault tolerance tests, as both constructs share similar parsing logic.
internal/specs_format/index.go (3)
Learnt from: ascandone
PR: formancehq/numscript#27
File: internal/interpreter/interpreter.go:667-668
Timestamp: 2024-12-05T11:42:58.472Z
Learning: In Go test files within the `internal/interpreter` package (e.g., `reconciler_test.go` and `interpreter_test.go`), it's acceptable to use hardcoded `"<kept>"` strings in test data and comments, and they do not need to be replaced with the `KEPT_ADDR` constant.
Learnt from: ascandone
PR: formancehq/numscript#45
File: internal/parser/ast.go:21-21
Timestamp: 2025-03-18T11:14:21.411Z
Learning: In Go, empty methods like `valueExpr()` are commonly used as interface markers and don't require dedicated unit tests as they have no functionality to test. The actual behavior is tested through integration tests that use the types in context.
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
🧬 Code Graph Analysis (10)
internal/interpreter/function_statements.go (2)
internal/utils/utils.go (1)
MapGetOrPutDefault(50-58)internal/interpreter/interpreter.go (1)
AccountMetadata(26-26)
internal/interpreter/balances_test.go (2)
internal/interpreter/interpreter.go (2)
Balances(24-24)AccountBalance(23-23)internal/interpreter/balances.go (1)
CompareBalances(98-102)
internal/parser/parser.go (1)
internal/parser/range.go (1)
Range(13-16)
internal/utils/pretty_csv_test.go (1)
internal/utils/pretty_csv.go (2)
CsvPretty(12-74)CsvPrettyMap(76-83)
internal/interpreter/interpreter.go (2)
internal/utils/utils.go (1)
MapGetOrPutDefault(50-58)internal/utils/pretty_csv.go (2)
CsvPretty(12-74)CsvPrettyMap(76-83)
internal/interpreter/accounts_metadata.go (3)
internal/interpreter/interpreter.go (2)
AccountsMetadata(27-27)AccountMetadata(26-26)internal/utils/utils.go (1)
MapGetOrPutDefault(50-58)internal/utils/pretty_csv.go (1)
CsvPretty(12-74)
internal/utils/pretty_csv.go (1)
internal/ansi/ansi.go (1)
ColorCyan(50-52)
internal/specs_format/run_test.go (2)
internal/specs_format/index.go (5)
Specs(14-20)Check(69-190)SpecsResult(48-54)TestCaseResult(37-46)AssertionMismatch(215-219)internal/interpreter/interpreter.go (4)
VariablesMap(15-15)Balances(24-24)AccountBalance(23-23)AccountsMetadata(27-27)
internal/interpreter/balances.go (3)
internal/interpreter/interpreter.go (2)
Balances(24-24)AccountBalance(23-23)internal/utils/utils.go (3)
NestedMapGetOrPutDefault(60-66)MapGetOrPutDefault(50-58)Map2Cmp(83-87)internal/utils/pretty_csv.go (1)
CsvPretty(12-74)
internal/cmd/run.go (1)
internal/interpreter/interpreter.go (3)
PrettyPrintPostings(996-1003)Metadata(78-78)PrettyPrintMeta(1005-1012)
🔇 Additional comments (48)
.gitignore (1)
7-8: Ignoring.direnvis the right callGood addition to keep local
direnvartefacts out of version control.internal/parser/parser.go (2)
14-21: Good refactoring to improve error handling.The anonymous embedding of
Rangeand addition of theError()method are solid improvements:
- Anonymous embedding allows direct access to Range fields without qualification
- Implementing the error interface makes ParserError more idiomatic
- The Error() method properly returns the error message
86-86: Consistent update following the embedding change.The method call correctly reflects the anonymous embedding of Range in ParserError.
internal/interpreter/batch_balances_query.go (1)
80-80: Method name updated to reflect exported interface.The change from
mergeBalancetoMergealigns with the method being exported in the balances.go file.internal/cmd/root.go (1)
28-28: Good integration of the new test command.The addition properly integrates the new testing functionality into the CLI following the established pattern.
internal/interpreter/function_statements.go (2)
3-6: Good consolidation of utility functions.Adding the utils import to use centralized utility functions instead of local implementations improves code organization and consistency.
31-33: Consistent use of centralized utility function.The change from local
defaultMapGettoutils.MapGetOrPutDefaultmaintains the same functionality while using the centralized utility function. The lambda function providing the default value is clean and readable.internal/interpreter/balances_test.go (4)
7-7: Good addition of snapshot testing capability.Adding the go-snaps import enables snapshot testing for the pretty-printing functionality, which is appropriate for testing formatted output.
45-45: Method name updated to reflect exported interface.The change from
deepClone()toDeepClone()aligns with the method being exported in the balances.go file.
52-64: Comprehensive test for pretty-printing functionality.The snapshot test for
PrettyPrint()provides good coverage for the formatted output functionality. The test data includes various account names and asset formats, which helps validate the formatting logic.
66-81: Effective test for balance comparison functionality.The test validates the
CompareBalancesfunction with differing balance values, ensuring the comparison logic works correctly. The test setup is clear and the assertion is appropriate.internal/interpreter/interpreter.go (2)
49-51: Good consolidation of utility functions.The replacement of direct map access with
utils.MapGetOrPutDefaultimproves consistency and reduces code duplication across the codebase.
605-605: Method name change suggests proper API design.The change from
deepClone()toDeepClone()indicates the method was exported, which is appropriate for a method that may be used by other packages.internal/interpreter/balances.go (2)
10-20: Good consolidation of utility functions.The replacement of internal helper functions with generic utilities from
utils.NestedMapGetOrPutDefaultimproves consistency and reduces code duplication.
72-83: Method export and consolidation improves API design.The renaming from
mergeBalancetoMergeand the use ofutils.MapGetOrPutDefaultcreates a cleaner, more consistent API.internal/interpreter/accounts_metadata.go (2)
7-11: LGTM: Clean helper method implementationThe
fetchAccountMetadatamethod correctly uses the new utility function to retrieve or create account metadata with proper default handling.
38-50: LGTM: Well-structured pretty-printing methodThe
PrettyPrintmethod correctly formats the metadata into a CSV-style output with appropriate headers and leverages the utility function for consistent formatting.internal/cmd/run.go (3)
23-31: LGTM: Improved naming clarityThe renaming from
ArgstorunArgsprovides better specificity and aligns with consistent naming patterns across CLI commands.
174-182: LGTM: Clean refactoring to use pretty-print utilitiesThe refactored
showPrettyfunction properly leverages the newPrettyPrintPostingsandPrettyPrintMetafunctions, simplifying the code by removing manual JSON marshaling and ANSI color handling. The conditional printing of metadata when non-empty is a good UX improvement.
186-186: LGTM: Consistent variable namingThe initialization of
runArgsaligns with the struct renaming and maintains consistency.internal/utils/utils.go (4)
50-58: LGTM: Well-designed generic utility functionThe
MapGetOrPutDefaultfunction provides a clean and reusable pattern for map access with default insertion. The generic implementation makes it versatile across different value types.
60-66: LGTM: Efficient nested map utilityThe
NestedMapGetOrPutDefaultfunction elegantly leverages the single-level function to handle nested maps, maintaining consistency and reducing code duplication.
68-81: LGTM: Solid map comparison implementationThe
MapCmpfunction correctly implements deep equality comparison for maps with custom comparison functions. The length check optimization and proper key existence validation are well-implemented.
83-87: LGTM: Clean composition of comparison functionsThe
Map2Cmpfunction effectively composes the single-level comparison to handle nested maps, maintaining the same interface pattern and leveraging existing functionality.internal/specs_format/run_test.go (6)
14-25: LGTM: Well-structured test programThe example program provides a clean test case with variables and send operations, suitable for testing the specs format functionality.
26-86: LGTM: Comprehensive simple test caseThe
TestRunSpecsSimpletest effectively validates the basic functionality of the specs format checker with proper JSON unmarshaling, execution, and result verification. The commented-out sections indicate thorough consideration of the expected structure.
88-157: LGTM: Thorough merging testThe
TestRunSpecsMergeOutertest excellently validates the merging behavior of outer and inner scopes for variables and balances, ensuring the specs format correctly handles inheritance and overrides.
159-210: LGTM: Important edge case coverageThe
TestRunWithMissingBalancetest properly validates the handling of insufficient funds scenarios and the correct assertion failure reporting whenexpect.missingFundsis false but funds are actually missing.
212-262: LGTM: Consistent failure handlingThe
TestRunWithMissingBalanceWhenExpectedPostingstest ensures that missing funds errors are properly detected and reported regardless of whether specific postings are expected.
264-308: LGTM: Null postings handlingThe
TestNullPostingsIsNooptest correctly validates that null expected postings work as intended with empty programs, ensuring the specs format handles this edge case properly.internal/ansi/ansi.go (4)
10-17: LGTM: Clean composition functionThe
Composefunction provides a clean way to chain multiple ANSI styling functions, following functional programming patterns effectively.
19-25: LGTM: Useful utility functionThe
replaceLastfunction correctly implements last occurrence replacement, which is essential for the color stacking functionality.
27-32: LGTM: Clever color stacking implementationThe modified
colfunction cleverly enables color stacking by inserting new color codes before the last reset, allowing multiple styles to be applied without premature resets. The TODO comment suggests this was intentionally designed for composition.
38-96: LGTM: Comprehensive color and styling additionsThe new color functions (white, green, light, bright variants) and background colors (dark, red, green) provide comprehensive styling options. The text modifiers (Bold, Underline) follow the same consistent pattern and use appropriate ANSI codes.
internal/cmd/test_test.go (7)
11-25: LGTM!The test correctly validates the diff display functionality using snapshot testing.
27-74: LGTM!The test properly validates single test execution with both passing and failing test cases.
76-128: LGTM!The test comprehensively validates complex assertions including balances, volumes, movements, and missing funds scenarios.
137-152: LGTM!The test correctly validates JSON parse error handling.
154-169: LGTM!The test correctly validates schema error handling for invalid specs format.
171-186: LGTM!The test correctly validates numscript parse error handling.
188-212: LGTM!The test correctly validates runtime error handling for invalid numscript execution.
internal/cmd/test.go (5)
116-162: LGTM!The function properly handles all error cases and provides clear output formatting for test results.
164-190: LGTM!The diff display correctly shows expected values in green and received values in red, which aligns with the testing context where expected values are what should be present.
192-239: LGTM!The function provides comprehensive failure reporting with clear formatting of given conditions and assertion mismatches.
241-252: LGTM!The function properly formats and displays interpreter errors with source code context.
332-367: LGTM!The command setup correctly implements the test subcommand with appropriate defaults and documentation.
internal/specs_format/index.go (2)
13-54: LGTM!The struct definitions provide a comprehensive and well-organized structure for test specifications and results.
192-213: LGTM!The merge functions correctly use deep cloning to avoid mutating original data structures.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/specs_format/runner.go (1)
75-75: Consider using the actual specs filename in the error messageThe error message assumes a
.specs.jsonsuffix, but the actual filename is available inrawSpec.SpecsPath. This could be misleading if the specs file has a different naming convention.- _, _ = stderr.Write([]byte(ansi.ColorRed(fmt.Sprintf("\nError: %s.specs.json\n\n", rawSpec.NumscriptPath)))) + _, _ = stderr.Write([]byte(ansi.ColorRed(fmt.Sprintf("\nError: %s\n\n", rawSpec.SpecsPath))))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/specs_format/__snapshots__/runner_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (3)
internal/cmd/test.go(1 hunks)internal/specs_format/runner.go(1 hunks)internal/specs_format/runner_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cmd/test.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ascandone
PR: formancehq/numscript#40
File: internal/lsp/codeactions_test.go:177-189
Timestamp: 2025-03-07T12:40:05.628Z
Learning: When reviewing code, focus on substantive issues related to core functionality rather than minor details in test utilities. For ascandone specifically, avoid nitpicky comments about testing utilities.
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.961Z
Learning: Edge cases for oneof destination parsing in numscript are covered by the existing inorder parsing fault tolerance tests, as both constructs share similar parsing logic.
Learnt from: ascandone
PR: formancehq/numscript#55
File: internal/parser/antlrParser/numscript_parser.go:2953-2967
Timestamp: 2025-04-22T16:45:07.842Z
Learning: In numscript, type constraints for color expressions are not enforced at the grammar level but are instead handled during static analysis or at runtime. The grammar is intentionally kept permissive (accepting any valueExpr after the RESTRICT token) to maintain separation of concerns.
Learnt from: ascandone
PR: formancehq/numscript#70
File: internal/utils/pretty_csv.go:12-16
Timestamp: 2025-07-16T14:49:54.326Z
Learning: ascandone prefers documenting function preconditions in comments rather than adding defensive validation code when the precondition violation would cause a panic. This is a valid design choice for utility functions.
internal/specs_format/runner_test.go (4)
Learnt from: ascandone
PR: formancehq/numscript#27
File: internal/interpreter/interpreter.go:667-668
Timestamp: 2024-12-05T11:42:58.472Z
Learning: In Go test files within the `internal/interpreter` package (e.g., `reconciler_test.go` and `interpreter_test.go`), it's acceptable to use hardcoded `"<kept>"` strings in test data and comments, and they do not need to be replaced with the `KEPT_ADDR` constant.
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
Learnt from: ascandone
PR: formancehq/numscript#45
File: internal/parser/ast.go:21-21
Timestamp: 2025-03-18T11:14:21.411Z
Learning: In Go, empty methods like `valueExpr()` are commonly used as interface markers and don't require dedicated unit tests as they have no functionality to test. The actual behavior is tested through integration tests that use the types in context.
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.961Z
Learning: Edge cases for oneof destination parsing in numscript are covered by the existing inorder parsing fault tolerance tests, as both constructs share similar parsing logic.
internal/specs_format/runner.go (2)
Learnt from: ascandone
PR: formancehq/numscript#27
File: internal/interpreter/interpreter.go:667-668
Timestamp: 2024-12-05T11:42:58.472Z
Learning: In Go test files within the `internal/interpreter` package (e.g., `reconciler_test.go` and `interpreter_test.go`), it's acceptable to use hardcoded `"<kept>"` strings in test data and comments, and they do not need to be replaced with the `KEPT_ADDR` constant.
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
🧬 Code Graph Analysis (1)
internal/specs_format/runner_test.go (1)
internal/specs_format/runner.go (3)
ShowDiff(111-137)RunSpecs(30-61)RawSpec(17-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor BugBot
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (9)
internal/specs_format/runner_test.go (9)
1-10: Package declaration and imports look good.The external test package naming and imports are appropriate for the testing functionality.
12-26: Test structure is clean and focused.The test appropriately validates the
ShowDifffunction with meaningful test data that includes both common elements and differences.
28-75: Comprehensive test covering both success and failure scenarios.The test effectively validates the basic functionality with realistic numscript content and specs, properly testing both passing and failing test cases.
77-129: Good coverage of complex assertion types.The test effectively covers advanced assertion types (balances, volumes, movements, missingFunds) with intentional mismatches to validate error reporting functionality.
131-136: Good edge case coverage for empty input.The test appropriately validates error handling when no specs files are provided.
138-153: Appropriate test for JSON parsing error handling.The test effectively validates error handling for invalid JSON specs content.
155-170: Good schema validation error test.The test appropriately validates schema validation error handling with valid JSON that doesn't conform to the expected schema.
172-187: Appropriate numscript parsing error test.The test effectively validates error handling for invalid numscript content.
189-213: Good runtime error handling test.The test appropriately validates runtime error handling with a numscript that will fail during execution due to invalid source format.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/cmd/test.go (2)
10-14: Consider alternatives to global variable for better testability.While the global
optsvariable works for this CLI command, consider passing arguments directly torunTestCmd()to improve testability and reduce implicit dependencies.-var opts = testArgs{} - -func runTestCmd() { +func runTestCmd(paths []string) {And update the caller:
- opts.paths = paths - runTestCmd() + runTestCmd(paths)
16-28: Well-structured test execution with proper error handling.The function correctly handles the test workflow and provides appropriate exit codes. The error handling and output stream usage are well-implemented.
The return statement after
os.Exit(1)is unreachable and can be removed:os.Exit(1) - return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/cmd/test.go(1 hunks)internal/specs_format/runner.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/specs_format/runner.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ascandone
PR: formancehq/numscript#40
File: internal/lsp/codeactions_test.go:177-189
Timestamp: 2025-03-07T12:40:05.628Z
Learning: When reviewing code, focus on substantive issues related to core functionality rather than minor details in test utilities. For ascandone specifically, avoid nitpicky comments about testing utilities.
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.961Z
Learning: Edge cases for oneof destination parsing in numscript are covered by the existing inorder parsing fault tolerance tests, as both constructs share similar parsing logic.
Learnt from: ascandone
PR: formancehq/numscript#55
File: internal/parser/antlrParser/numscript_parser.go:2953-2967
Timestamp: 2025-04-22T16:45:07.842Z
Learning: In numscript, type constraints for color expressions are not enforced at the grammar level but are instead handled during static analysis or at runtime. The grammar is intentionally kept permissive (accepting any valueExpr after the RESTRICT token) to maintain separation of concerns.
Learnt from: ascandone
PR: formancehq/numscript#70
File: internal/utils/pretty_csv.go:12-16
Timestamp: 2025-07-16T14:49:54.326Z
Learning: ascandone prefers documenting function preconditions in comments rather than adding defensive validation code when the precondition violation would cause a panic. This is a valid design choice for utility functions.
internal/cmd/test.go (1)
Learnt from: ascandone
PR: formancehq/numscript#27
File: internal/interpreter/interpreter.go:667-668
Timestamp: 2024-12-05T11:42:58.472Z
Learning: In Go test files within the `internal/interpreter` package (e.g., `reconciler_test.go` and `interpreter_test.go`), it's acceptable to use hardcoded `"<kept>"` strings in test data and comments, and they do not need to be replaced with the `KEPT_ADDR` constant.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor BugBot
- GitHub Check: Tests
🔇 Additional comments (2)
internal/cmd/test.go (2)
1-8: Clean package structure and appropriate imports.The imports are well-organized and include only the necessary dependencies for the CLI command functionality.
30-51: Excellent command structure with clear documentation.The cobra command is well-configured with helpful documentation, appropriate argument handling, and sensible defaults. The file naming convention is clearly explained in the Long description.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/specs_format/index.go (1)
221-221: Address the TODO comment for missing tests.The
Movementstype and its associatedgetMovementsfunction handle complex nested map operations and should have comprehensive unit tests to ensure correctness.Would you like me to help generate unit tests for the
Movementstype andgetMovementsfunction, or open a new issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
specs.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
internal/specs_format/index.go(1 hunks)internal/specs_format/parse_test.go(1 hunks)internal/specs_format/run_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/specs_format/parse_test.go
- internal/specs_format/run_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ascandone
PR: formancehq/numscript#40
File: internal/lsp/codeactions_test.go:177-189
Timestamp: 2025-03-07T12:40:05.628Z
Learning: When reviewing code, focus on substantive issues related to core functionality rather than minor details in test utilities. For ascandone specifically, avoid nitpicky comments about testing utilities.
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.961Z
Learning: Edge cases for oneof destination parsing in numscript are covered by the existing inorder parsing fault tolerance tests, as both constructs share similar parsing logic.
Learnt from: ascandone
PR: formancehq/numscript#55
File: internal/parser/antlrParser/numscript_parser.go:2953-2967
Timestamp: 2025-04-22T16:45:07.842Z
Learning: In numscript, type constraints for color expressions are not enforced at the grammar level but are instead handled during static analysis or at runtime. The grammar is intentionally kept permissive (accepting any valueExpr after the RESTRICT token) to maintain separation of concerns.
Learnt from: ascandone
PR: formancehq/numscript#70
File: internal/utils/pretty_csv.go:12-16
Timestamp: 2025-07-16T14:49:54.326Z
Learning: ascandone prefers documenting function preconditions in comments rather than adding defensive validation code when the precondition violation would cause a panic. This is a valid design choice for utility functions.
internal/specs_format/index.go (2)
Learnt from: ascandone
PR: formancehq/numscript#27
File: internal/interpreter/interpreter.go:667-668
Timestamp: 2024-12-05T11:42:58.472Z
Learning: In Go test files within the `internal/interpreter` package (e.g., `reconciler_test.go` and `interpreter_test.go`), it's acceptable to use hardcoded `"<kept>"` strings in test data and comments, and they do not need to be replaced with the `KEPT_ADDR` constant.
Learnt from: ascandone
PR: formancehq/numscript#45
File: internal/parser/ast.go:21-21
Timestamp: 2025-03-18T11:14:21.411Z
Learning: In Go, empty methods like `valueExpr()` are commonly used as interface markers and don't require dedicated unit tests as they have no functionality to test. The actual behavior is tested through integration tests that use the types in context.
🧬 Code Graph Analysis (1)
internal/specs_format/index.go (5)
internal/interpreter/interpreter.go (8)
Balances(24-24)VariablesMap(15-15)AccountsMetadata(27-27)Posting(80-85)InterpreterError(73-76)RunProgram(228-285)StaticStore(34-37)Metadata(78-78)internal/parser/ast.go (2)
Program(325-329)Destination(189-192)internal/interpreter/interpreter_error.go (1)
MissingFundsErr(10-15)internal/interpreter/balances.go (1)
CompareBalances(98-102)internal/utils/utils.go (2)
NestedMapGetOrPutDefault(60-66)MapGetOrPutDefault(50-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor BugBot
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (4)
internal/specs_format/index.go (4)
13-55: Well-designed data structures with clear separation of concerns.The structs appropriately separate global specs configuration from per-test case inputs and expectations. The JSON tags with
omitemptyare correctly applied for optional fields, and the invariant documentation inSpecsResultis helpful.
56-67: Clean generic assertion function with flexible comparison.The function design allows for different comparison strategies while maintaining type safety. The logic correctly appends failed assertions only when the comparison fails.
69-190: Comprehensive test execution with proper error handling and assertion logic.The function correctly:
- Merges global and per-test configurations
- Handles different error types, specifically distinguishing
MissingFundsErr- Uses appropriate comparison functions for different data types (
reflect.DeepEqualvsinterpreter.CompareBalances)- Converts metadata values to strings for comparison
- Tracks both summary counts and detailed results
192-213: Correct merge implementations with proper precedence and immutability.The merge functions appropriately handle different data types:
- Simple map copying for variables
- Deep cloning for complex types to prevent mutation
- Per-test values correctly take precedence over global values
| func getMovements(postings []interpreter.Posting) Movements { | ||
| m := Movements{} | ||
|
|
||
| for _, posting := range postings { | ||
| assetsMap := utils.NestedMapGetOrPutDefault(m, posting.Source, posting.Destination, func() map[string]*big.Int { | ||
| return map[string]*big.Int{} | ||
| }) | ||
|
|
||
| amt := utils.MapGetOrPutDefault(assetsMap, posting.Asset, func() *big.Int { | ||
| return new(big.Int) | ||
| }) | ||
|
|
||
| amt.Add(amt, posting.Amount) | ||
| } | ||
|
|
||
| return m | ||
| } | ||
|
|
||
| func getVolumes(postings []interpreter.Posting, initialBalances interpreter.Balances) interpreter.Balances { | ||
| balances := initialBalances.DeepClone() | ||
| for _, posting := range postings { | ||
| sourceBalance := utils.NestedMapGetOrPutDefault(balances, posting.Source, posting.Asset, func() *big.Int { | ||
| return new(big.Int) | ||
| }) | ||
| sourceBalance.Sub(sourceBalance, posting.Amount) | ||
|
|
||
| destinationBalance := utils.NestedMapGetOrPutDefault(balances, posting.Destination, posting.Asset, func() *big.Int { | ||
| return new(big.Int) | ||
| }) | ||
| destinationBalance.Add(destinationBalance, posting.Amount) | ||
| } | ||
|
|
||
| return balances | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the correctness of movement and volume calculations.
The functions implement double-entry bookkeeping logic:
getMovementsaccumulates amounts by source→destination→assetgetVolumesapplies postings to initial balances (subtract from source, add to destination)
While the logic appears correct, these financial calculations should be thoroughly tested.
Consider adding unit tests to verify these calculations with various scenarios including:
- Multiple postings between same accounts
- Different assets
- Edge cases with zero amounts
🏁 Script executed:
#!/bin/bash
# Search for existing tests of these functions
rg -A 10 -B 2 "getMovements|getVolumes" --type goLength of output: 2968
Update test coverage for getMovements and getVolumes
Tests for these functions already exist in internal/specs_format/index.go (see calls to getMovements and getVolumes with ExpectMovements/ExpectVolumes). Please review and, if any scenario is not covered, add focused unit tests covering:
- Multiple postings between the same accounts
- Transactions involving different assets
- Edge cases such as zero amounts
• File to inspect:
- internal/specs_format/index.go (around the
ExpectMovements/ExpectVolumestest blocks)
🤖 Prompt for AI Agents
In internal/specs_format/index.go around the existing test blocks for
ExpectMovements and ExpectVolumes, review current unit tests for getMovements
and getVolumes to identify any missing scenarios. Add focused tests covering
multiple postings between the same accounts, transactions involving different
assets, and edge cases like zero amounts to ensure comprehensive coverage of
these functions.
2 participants
Add a
numscript testcommand.(note for reviewers: do not read commit-by-commit, read final diff directly)