test(tuplefile): add unit tests for CSV and JSONL parsing#664
test(tuplefile): add unit tests for CSV and JSONL parsing#664archy-rock3t-cloud wants to merge 1 commit intoopenfga:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a comprehensive Go test suite for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Adds a new tuplefile test suite to validate tuple parsing behavior for CSV and JSONL inputs, including optional condition fields and common error cases.
Changes:
- Adds CSV header parsing tests (required headers, invalid headers, empty input, condition header dependency).
- Adds CSV row parsing tests (user relations, conditions, invalid condition context, headers-only input).
- Adds JSONL parsing tests (valid lines, blank lines, conditions, invalid JSON, empty input).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var tuples []client.ClientTupleKey | ||
| err := parseTuplesFromCSV([]byte(csv), &tuples) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "failed to read condition context on line 0") |
There was a problem hiding this comment.
This assertion hard-codes the line number "0" in the error message, which is an implementation detail and may be semantically confusing (CSV files are typically 1-based and include the header line). Consider asserting only the stable part of the message (e.g., "failed to read condition context") or aligning the code/test to a consistent 1-based line numbering scheme.
| assert.Contains(t, err.Error(), "failed to read condition context on line 0") | |
| assert.Contains(t, err.Error(), "failed to read condition context") |
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| var tuples []client.ClientTupleKey | ||
| err := parseTuplesFromCSV([]byte(tc.csv), &tuples) | ||
| require.NoError(t, err) | ||
| }) |
There was a problem hiding this comment.
The subtests call t.Parallel() while ranging over cases without capturing the loop variable. This can make subtests read the wrong tc values (flaky/incorrect). Capture tc := tc inside the loop (or avoid t.Parallel in subtests).
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| var tuples []client.ClientTupleKey | ||
| err := parseTuplesFromCSV([]byte(tc.csv), &tuples) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), tc.expectedErr) | ||
| }) |
There was a problem hiding this comment.
Same loop-variable capture issue here: subtests run in parallel but tc isn't re-bound inside the for _, tc := range cases loop, so assertions may use the wrong case data.
| cases := []struct { | ||
| name string | ||
| csv string | ||
| expected csvColumns | ||
| }{ | ||
| { | ||
| name: "minimal required headers", | ||
| csv: "user_type,user_id,relation,object_type,object_id\n", | ||
| expected: csvColumns{ | ||
| UserType: 0, | ||
| UserID: 1, | ||
| UserRelation: -1, | ||
| Relation: 2, | ||
| ObjectType: 3, | ||
| ObjectID: 4, | ||
| ConditionName: -1, | ||
| ConditionContext: -1, | ||
| }, | ||
| }, | ||
| { | ||
| name: "all headers present", | ||
| csv: "user_type,user_id,user_relation,relation,object_type,object_id,condition_name,condition_context\n", | ||
| expected: csvColumns{ | ||
| UserType: 0, | ||
| UserID: 1, | ||
| UserRelation: 2, | ||
| Relation: 3, | ||
| ObjectType: 4, | ||
| ObjectID: 5, | ||
| ConditionName: 6, | ||
| ConditionContext: 7, | ||
| }, | ||
| }, | ||
| { | ||
| name: "headers in different order", | ||
| csv: "object_id,relation,user_id,object_type,user_type\n", | ||
| expected: csvColumns{ | ||
| UserType: 4, | ||
| UserID: 2, | ||
| UserRelation: -1, | ||
| Relation: 1, | ||
| ObjectType: 3, | ||
| ObjectID: 0, | ||
| ConditionName: -1, | ||
| ConditionContext: -1, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| var tuples []client.ClientTupleKey | ||
| err := parseTuplesFromCSV([]byte(tc.csv), &tuples) | ||
| require.NoError(t, err) | ||
| }) |
There was a problem hiding this comment.
This test defines an expected csvColumns for each case but never asserts it, so it only verifies that parsing doesn't error. Consider calling readHeaders directly (or otherwise exposing the parsed header mapping) and asserting the returned csvColumns matches tc.expected to actually test header indexing/order handling.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/tuplefile/tuplefile_test.go (2)
17-21:expectedheader mapping fixture is currently unused.
tc.expectedis populated (e.g., Line 20 and case bodies) but never asserted in the test loop (Lines 66-73). Right now this block only verifies “no error”, so header-index mapping regressions could slip through unnoticed. Consider either asserting header mapping behavior explicitly or removingexpectedto avoid misleading test intent.Also applies to: 66-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tuplefile/tuplefile_test.go` around lines 17 - 21, The test defines an unused expected header mapping (tc.expected of type csvColumns) but never asserts it; update the test loop in tuplefile_test.go to compare the actual parsed header mapping to tc.expected (or if intended, remove the expected field from the test cases). Specifically, locate where the test calls the parser and checks for no error (the test loop that references tc.csv and tc.name), retrieve the produced header mapping from the parser result, and add an assertion that it equals tc.expected (using the same equality/assert helper used elsewhere in the file), or delete the expected field and its per-case values to avoid misleading fixtures.
252-252: Prefer stable error assertions over exact line-number text.The checks on Line 252 and Line 330 hardcode parser line numbers in error messages. That makes tests fragile to harmless wording/indexing changes. Consider asserting a stable substring (e.g., error category/prefix) instead of exact line suffixes.
Also applies to: 330-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tuplefile/tuplefile_test.go` at line 252, Replace brittle assertions that match exact line-number text by checking a stable error substring; specifically, change assertions like assert.Contains(t, err.Error(), "failed to read condition context on line 0") to assert.Contains(t, err.Error(), "failed to read condition context") (and similarly for the other assertion that hardcodes a line number). This targets the error message text used by the parser (the "failed to read condition context on line X" string) and ensures tests assert the error category/prefix rather than a fragile line-number suffix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/tuplefile/tuplefile_test.go`:
- Around line 17-21: The test defines an unused expected header mapping
(tc.expected of type csvColumns) but never asserts it; update the test loop in
tuplefile_test.go to compare the actual parsed header mapping to tc.expected (or
if intended, remove the expected field from the test cases). Specifically,
locate where the test calls the parser and checks for no error (the test loop
that references tc.csv and tc.name), retrieve the produced header mapping from
the parser result, and add an assertion that it equals tc.expected (using the
same equality/assert helper used elsewhere in the file), or delete the expected
field and its per-case values to avoid misleading fixtures.
- Line 252: Replace brittle assertions that match exact line-number text by
checking a stable error substring; specifically, change assertions like
assert.Contains(t, err.Error(), "failed to read condition context on line 0") to
assert.Contains(t, err.Error(), "failed to read condition context") (and
similarly for the other assertion that hardcodes a line number). This targets
the error message text used by the parser (the "failed to read condition context
on line X" string) and ensures tests assert the error category/prefix rather
than a fragile line-number suffix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1c6fb81-645d-4dee-ad9f-ea78e3a4d09f
📒 Files selected for processing (1)
internal/tuplefile/tuplefile_test.go
Signed-off-by: Artem Muterko <artem@sopho.tech>
archy-rock3t-cloud
force-pushed
the
tuplefile-parsing-tests
branch
from
2db58a8 to
ba87fa9
Compare
|
Thanks @copilot / @coderabbitai — addressed the review:
Rebased on The remaining |
|
Triggering a fresh review of the updated branch. ✅ Actions performedReview triggered.
|
Add unit tests for CSV and JSONL
parsing
Summary by CodeRabbit