Add glob support for test command #515
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds glob pattern support for the test command, allowing users to specify multiple test files using wildcard patterns. Key changes include:
- Replacing a single file read with glob expansion to accept patterns.
- Updating error messages to include the file name when a test file fails.
- Documenting the new globbing support in the README and CHANGELOG.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/model/test.go | Uses filepath.Glob to expand test file patterns and aggregates test results. |
| README.md | Updates test command documentation to reflect support for glob patterns. |
| CHANGELOG.md | Documents the addition of glob support for test files. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/model/test.go (1)
51-57: Well-implemented glob pattern handling with good fallback behavior.The implementation correctly uses
filepath.Glob()and provides a sensible fallback when no files match the pattern, maintaining backward compatibility.Consider adding a user-friendly message when falling back to single file mode to help users understand the behavior:
if len(fileNames) == 0 { + fmt.Fprintf(os.Stderr, "No files matched the pattern '%s'. Treating as single file name.\n", testsFileName) fileNames = []string{testsFileName} }
🧹 Nitpick comments (1)
cmd/model/test.go (1)
113-113: Consider updating the flag description to mention glob pattern support.The flag description should reflect the new glob pattern capability.
- testCmd.Flags().String("tests", "", "Tests file Name. The file should have the OpenFGA tests in a valid YAML or JSON format") //nolint:lll + testCmd.Flags().String("tests", "", "Tests file name or glob pattern. Files should have OpenFGA tests in a valid YAML or JSON format") //nolint:lll
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)README.md(3 hunks)cmd/model/test.go(3 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~528-~528: Loose punctuation mark.
Context: ...test ###### Parameters * --tests: Name of the tests file, or a glob patte...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~528-~528: Possible missing comma found.
Context: ...r a glob pattern to multiple files (for example tests/*.fga.yaml). Each file must be ...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
README.md
528-528: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
🔇 Additional comments (6)
CHANGELOG.md (1)
9-9: Verify the correct PR number reference.The changelog references PR #423, but the PR objectives indicate this is PR #515. Please ensure the correct PR number is referenced.
README.md (1)
381-381: Documentation accurately reflects the new glob support feature.The updates clearly document that the
--testsparameter now accepts both single YAML test files and glob patterns. The examples are consistently updated throughout the documentation.Also applies to: 528-528, 607-607
cmd/model/test.go (4)
23-23: Good addition of filepath import for glob functionality.The import is necessary for the new glob pattern support.
59-59: Good initialization of aggregate results structure.Proper setup for collecting results from multiple test files.
71-87: Excellent implementation of multi-file test execution and aggregation.The loop correctly processes each file, runs tests, and aggregates results. Error handling includes file-specific context which is helpful for debugging failures.
89-99: Proper use of aggregated results for final output and status determination.The implementation correctly uses the aggregated results for both verbose output and summary display, ensuring all test results are considered in the final pass/fail determination.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/model/test.go (1)
52-58: Consider adding user feedback for fallback behavior.The glob resolution logic is correct, but users might be confused when no files match their pattern and the command silently falls back to treating the pattern as a single filename.
Consider adding a warning message to clarify the fallback behavior:
if len(fileNames) == 0 { + fmt.Fprintf(os.Stderr, "No files matched the pattern '%s'. Treating it as a single file name.\n", testsFileName) fileNames = []string{testsFileName} }
🧹 Nitpick comments (1)
cmd/model/test.go (1)
93-94: Consider a more robust approach for header removal.The string replacement to remove the test summary header is fragile and could break if the format of
FriendlyDisplay()changes.Consider using a more robust approach:
- summaryText := strings.Replace(test.FriendlyDisplay(), "# Test Summary #\n", "", 1) - summary := fmt.Sprintf("# file: %s\n%s", file, summaryText) + summaryText := test.FriendlyDisplay() + if strings.HasPrefix(summaryText, "# Test Summary #\n") { + summaryText = summaryText[len("# Test Summary #\n"):] + } + summary := fmt.Sprintf("# file: %s\n%s", file, summaryText)Or better yet, consider adding a method to
TestResultsthat returns the summary without the header, making the intent more explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/model/test.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Tests
- GitHub Check: Test Release Process
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (actions)
🔇 Additional comments (4)
cmd/model/test.go (4)
23-24: LGTM - Appropriate imports for the new functionality.The addition of
path/filepathfor glob support andstringsfor string manipulation are correctly imported and necessary for the new features.
60-63: LGTM - Clean setup for multi-file processing.The variables are well-named and appropriately initialized for tracking multiple files and aggregating results.
75-97: LGTM - Robust multi-file processing with good error handling.The loop correctly processes each file individually while maintaining proper error context. The conditional summary collection for multiple files is well-implemented.
99-115: LGTM - Excellent user experience for multi-file results.The aggregate results handling is well-designed:
- Uses aggregate results for final pass/fail determination
- Shows individual file summaries followed by aggregate summary for multiple files
- Maintains backward compatibility for single file usage
…fmt.Errorf("…: %w", err)), remove all // nolint:wrapcheck.
… of RunE in a single block.
…yBody() instead of stripping # Test Summary #.
…, so cancellations work
… instead of stripping # Test Summary #
…odel as modelTestCmd to avoid ambiguity.
….Stat() on the literal path and error out if it doesn't exist, otherwise use it.
….Stat() on the literal path and error out if it doesn't exist, otherwise use it.
…if --verbose dump the raw JSON.
|
Looking forward to this! It's really a pain to have to run all your tests files individually. It's easy enough to write a loop in your build pipelines but making sure you catch all tests when doing local development becomes a pain in the current state. This will simplify things on both ends tremendously! |
Summary
model testTesting
go test ./... -run=^$make test(fails: Interrupt)Fixes #423
https://chatgpt.com/codex/tasks/task_e_684859e0ba448322b5cf1c04ccc5aa1f
Sample Output
Summary by CodeRabbit
Summary by CodeRabbit
New Features
fga model testcommand with multiple test files using glob patterns.Documentation
--testsparameter accepts glob patterns to specify multiple YAML test files.