Refactored the cmd for leaderboard #129
Conversation
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
WalkthroughThis pull request introduces significant changes across multiple files, primarily adding new functionality for formatting nodes as JSON through the creation of a Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@naveensrinivasan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (13)
cmd/leaderboard/leaderboard.go (1)
18-19: Enhance the Long descriptionWhile the Short description is good, the Long description should provide more detailed information about the command's functionality and available subcommands.
- Long: `Commands to display and sort leaderboard data`, + Long: `Commands to display and sort leaderboard data. + +Available Commands: + - keys: Display all available leaderboard keys + - custom: Create custom leaderboard sorts`,cmd/helpers/formatter.go (3)
11-16: Add documentation for the nodeOutput structWhile the struct is well-designed, it would benefit from documentation explaining its purpose and fields, especially since it's in a helpers package that others might use.
Add documentation like this:
+// nodeOutput represents the JSON output format for a node. +// It contains the essential node information and optional metadata. type nodeOutput struct { Name string `json:"name"` Type string `json:"type"` ID string `json:"id"` Metadata map[string]interface{} `json:"metadata,omitempty"` }
18-19: Enhance function documentationThe current documentation is minimal. Consider adding more details about parameters, return values, and error conditions.
Add comprehensive documentation:
-// FormatNodeJSON formats the nodes as JSON. +// FormatNodeJSON formats a slice of nodes into a pretty-printed JSON format. +// It processes node metadata and converts internal representations to a consistent output format. +// +// Parameters: +// - nodes: A slice of node pointers to format. Must not be nil or empty. +// +// Returns: +// - []byte: Pretty-printed JSON representation of the nodes +// - error: Error if nodes are nil/empty, or if metadata unmarshaling fails func FormatNodeJSON(nodes []*v1.Node) ([]byte, error) {
28-43: Add validation for required node fieldsThe function should validate that required node fields (Name, Type, Id) are not empty before processing.
Consider adding validation:
outputs := make([]nodeOutput, 0, len(nodes)) for _, node := range nodes { + if node.Name == "" || node.Type == "" { + return nil, fmt.Errorf("node missing required fields: name=%q, type=%q", node.Name, node.Type) + } var metadata map[string]interface{} if len(node.Metadata) > 0 {cmd/helpers/formatter_test.go (2)
12-87: Consider adding a test case for multiple nodes.The test cases are well-structured and cover important scenarios including edge cases and error conditions. However, consider adding a test case that validates the formatting of multiple nodes in a single slice to ensure the array handling works correctly.
Here's a suggested additional test case:
{ name: "Valid node with invalid metadata", // ... existing test case ... }, + { + name: "Multiple valid nodes", + nodes: []*v1.Node{ + { + Name: "Node4", + Type: "TypeD", + Id: 4, + }, + { + Name: "Node5", + Type: "TypeE", + Id: 5, + Metadata: []byte(`{"key":"value"}`), + }, + }, + expectErr: false, + expected: `[ + { + "name": "Node4", + "type": "TypeD", + "id": "4" + }, + { + "name": "Node5", + "type": "TypeE", + "id": "5", + "metadata": { + "key": "value" + } + } + ]`, + },
89-120: Consider enhancing test failure messages and JSON comparison readability.While the test execution logic is solid, consider these improvements for better debugging experience:
if !strings.Contains(err.Error(), tt.expectedErr) { - t.Errorf("Expected error to contain '%s', but got '%s'", tt.expectedErr, err.Error()) + t.Errorf("[%s] Expected error to contain '%s', but got '%s'", tt.name, tt.expectedErr, err.Error()) } } else { if err != nil { - t.Errorf("Unexpected error: %v", err) + t.Errorf("[%s] Unexpected error: %v", tt.name, err) } var expectedBuffer bytes.Buffer if err := json.Compact(&expectedBuffer, []byte(tt.expected)); err != nil { - t.Fatalf("Invalid expected JSON: %v", err) + t.Fatalf("[%s] Invalid expected JSON: %v", tt.name, err) } var outputBuffer bytes.Buffer if err := json.Compact(&outputBuffer, output); err != nil { - t.Fatalf("Invalid output JSON: %v", err) + t.Fatalf("[%s] Invalid output JSON: %v", tt.name, err) } if expectedBuffer.String() != outputBuffer.String() { - t.Errorf("Expected output %s, got %s", expectedBuffer.String(), outputBuffer.String()) + t.Errorf("[%s]\nExpected:\n%s\nGot:\n%s", + tt.name, + formatJSON(expectedBuffer.String()), + formatJSON(outputBuffer.String())) } } +// Add this helper function at the end of the file +func formatJSON(input string) string { + var pretty bytes.Buffer + if err := json.Indent(&pretty, []byte(input), "", " "); err != nil { + return input // fallback to compact form if pretty print fails + } + return pretty.String() +}These changes will:
- Include the test case name in error messages for easier identification
- Pretty print JSON in diff messages for better readability
- Make test failure debugging more straightforward
cmd/leaderboard/keys/keys.go (2)
18-33: Consider enhancing flag handling and validation.While the flag setup is generally good, consider these improvements:
- Add validation for the output format during flag parsing
- Support environment variables for the server address
- Add validation for negative maxOutput values
Here's a suggested implementation:
func (o *options) AddFlags(cmd *cobra.Command) { - cmd.Flags().IntVarP(&o.maxOutput, "max-output", "m", 10, "Specify the maximum number of keys to display") + cmd.Flags().IntVarP(&o.maxOutput, "max-output", "m", 10, "Specify the maximum number of keys to display (must be positive)") + cmd.MarkFlagRequired("max-output") + cmd.MarkFlagPositive("max-output") cmd.Flags().BoolVarP(&o.showInfo, "show-info", "i", true, "Toggle display of additional information for each key") - cmd.Flags().StringVarP(&o.addr, "addr", "a", "http://localhost:8089", "Address of the Minefield server") + cmd.Flags().StringVarP(&o.addr, "addr", "a", "http://localhost:8089", "Address of the Minefield server (env: MINEFIELD_SERVER)") + if serverAddr := os.Getenv("MINEFIELD_SERVER"); serverAddr != "" { + o.addr = serverAddr + } - cmd.Flags().StringVarP(&o.output, "output", "o", "table", "Output format (table or json)") + cmd.Flags().StringVarP(&o.output, "output", "o", "table", "Output format (must be 'table' or 'json')") + cmd.RegisterFlagCompletionFunc("output", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + return []string{"table", "json"}, cobra.ShellCompDirectiveNoFileComp + }) }
104-153: Consider enhancing table rendering for better scalability and customization.The table rendering logic is solid but could benefit from these improvements:
- Move maxOutput validation to flag parsing
- Add table customization options (e.g., alignment, borders)
- Consider streaming for large responses
Here's a suggested enhancement:
func (o *options) renderTable(w io.Writer, resp *connect.Response[v1.AllKeysResponse], showInfo bool, maxOutput int) error { // ... existing const declarations ... table := tablewriter.NewWriter(w) table.SetAutoWrapText(false) table.SetRowLine(true) + // Enhanced table customization + table.SetAlignment(tablewriter.ALIGN_LEFT) + table.SetBorder(true) + table.SetRowSeparator("-") + table.SetColumnSeparator("|") // ... existing header setup ... + // Process rows in chunks for better memory usage + const chunkSize = 100 + processed := 0 for index, node := range resp.Msg.Nodes { if index >= maxOutput { break } + processed++ row := []string{ node.Name, node.Type, strconv.Itoa(int(node.Id)), } if showInfo { additionalInfo := helpers.ComputeAdditionalInfo(node) row = append(row, additionalInfo) } table.Append(row) + + // Render in chunks to avoid memory issues with large datasets + if processed%chunkSize == 0 { + table.Render() + table.ClearRows() + } } - table.Render() + // Render any remaining rows + if processed%chunkSize != 0 { + table.Render() + } return nil }cmd/leaderboard/keys/keys_test.go (5)
20-103: Enhance test coverage with additional test casesWhile the current test cases are good, consider adding these scenarios for more comprehensive coverage:
- Empty nodes array
- Very long node names/types that might affect table formatting
- Special characters in node names
- Edge case for maxOutput=0
- More thorough validation of table formatting (column alignment, spacing)
tests := []struct { name string nodes []*v1.Node showInfo bool maxOutput int expectedOutput []string wantErr bool }{ + { + name: "empty nodes array", + nodes: []*v1.Node{}, + showInfo: false, + maxOutput: 10, + expectedOutput: []string{ + "NAME", "TYPE", "ID", + }, + }, + { + name: "long node names", + nodes: []*v1.Node{ + {Name: "very_long_node_name_that_might_affect_formatting", Type: "type1", Id: 1}, + }, + showInfo: false, + maxOutput: 10, + expectedOutput: []string{ + "NAME", "TYPE", "ID", + "very_long_node_name_that_might_affect_formatting", + }, + }, }
173-205: Consider expanding command configuration testsThe test could be enhanced by verifying additional command properties:
- Command aliases
- Hidden status
- Deprecated status
- Parent/child relationship
- Flag persistence
if cmd.Long == "" { t.Error("Long description should not be empty") } +// Check command properties +if len(cmd.Aliases) > 0 { + t.Errorf("Expected no aliases, got %v", cmd.Aliases) +} + +if cmd.Hidden { + t.Error("Command should not be hidden") +} + +if cmd.Deprecated != "" { + t.Errorf("Command should not be deprecated, got %q", cmd.Deprecated) +} + // Check if the RunE function is set
207-281: Add flag validation testsConsider adding tests for flag validation:
- Invalid output format values
- Negative max-output values
- Invalid addr format
- Flag dependencies or mutual exclusions if any
func TestOptions_AddFlags(t *testing.T) { o := &options{} cmd := &cobra.Command{} o.AddFlags(cmd) + // Test flag validation + tests := []struct { + name string + setValue string + flagName string + shouldError bool + }{ + { + name: "invalid output format", + setValue: "invalid", + flagName: "output", + shouldError: true, + }, + { + name: "negative max-output", + setValue: "-1", + flagName: "max-output", + shouldError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := cmd.Flags().Set(tt.flagName, tt.setValue) + if (err != nil) != tt.shouldError { + t.Errorf("expected error: %v, got: %v", tt.shouldError, err) + } + }) + }
283-381: Add timeout and context cancellation testsThe mock implementation is good, but consider adding these test scenarios:
- Context timeout
- Context cancellation
- Different output formats (table vs json)
- Network-related errors
- Rate limiting scenarios
tests := []struct { name string args []string setupClient func() apiv1connect.LeaderboardServiceClient expectedError bool checkOutput func(output string) bool }{ + { + name: "Context timeout", + args: []string{}, + setupClient: func() apiv1connect.LeaderboardServiceClient { + return &MockLeaderboardServiceClient{ + AllKeysFunc: func(ctx context.Context, req *connect.Request[emptypb.Empty]) (*connect.Response[v1.AllKeysResponse], error) { + <-ctx.Done() + return nil, ctx.Err() + }, + } + }, + expectedError: true, + }, + { + name: "JSON output format", + args: []string{}, + setupClient: func() apiv1connect.LeaderboardServiceClient { + return &MockLeaderboardServiceClient{ + AllKeysFunc: func(ctx context.Context, req *connect.Request[emptypb.Empty]) (*connect.Response[v1.AllKeysResponse], error) { + return connect.NewResponse(&v1.AllKeysResponse{ + Nodes: []*v1.Node{{Name: "test", Type: "type", Id: 1}}, + }), nil + }, + } + }, + expectedError: false, + checkOutput: func(output string) bool { + return strings.Contains(output, `{"name":"test","type":"type","id":1}`) + }, + },
1-381: Consider organizing tests into sub-test packagesThe test file has grown quite large with multiple test functions. Consider:
- Moving mock implementations to a separate
testingpackage- Grouping related tests into logical test suites
- Adding package-level documentation about the testing strategy
- Creating test helpers for common setup/teardown operations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
cmd/helpers/formatter.go(1 hunks)cmd/helpers/formatter_test.go(1 hunks)cmd/leaderboard/allKeys/keys.go(0 hunks)cmd/leaderboard/keys/keys.go(1 hunks)cmd/leaderboard/keys/keys_test.go(1 hunks)cmd/leaderboard/leaderboard.go(2 hunks)cmd/leaderboard/weightedNACD/defaultWeights.json(0 hunks)cmd/leaderboard/weightedNACD/weightedNACD.go(0 hunks)cmd/query/globsearch/globsearch.go(2 hunks)
💤 Files with no reviewable changes (3)
- cmd/leaderboard/allKeys/keys.go
- cmd/leaderboard/weightedNACD/defaultWeights.json
- cmd/leaderboard/weightedNACD/weightedNACD.go
🔇 Additional comments (11)
cmd/leaderboard/leaderboard.go (2)
26-26: Verify keys command implementation
The keys.New() doesn't take a storage parameter unlike other commands. Let's verify that the keys command can function correctly without storage access.
#!/bin/bash
# Description: Check the implementation of keys command
# Look for storage usage in keys command implementation
ast-grep --pattern 'func New() *cobra.Command {
$$$
}'
# Check if there are any storage-related interfaces or methods in the keys package
rg -l "Storage" cmd/leaderboard/keys/5-5: Verify the removal of weightedNACD functionality
The removal of the weightedNACD import suggests this functionality has been removed or relocated. Let's verify this change doesn't cause any regressions.
cmd/helpers/formatter.go (2)
1-9: LGTM! Clean package structure and imports
The package name aligns with the directory structure, and imports are minimal and necessary.
45-45: LGTM! Proper use of MarshalIndent
Using json.MarshalIndent with appropriate indentation parameters ensures readable JSON output.
cmd/query/globsearch/globsearch.go (2)
10-10: LGTM! Clean import of helpers package.
The addition of the helpers package import is clean and follows Go conventions.
Line range hint 62-66: Verify the behavior of the centralized FormatNodeJSON helper.
The refactoring to use the centralized helpers.FormatNodeJSON looks good and maintains proper error handling. However, let's verify that the helper function maintains the same behavior as the previous implementation.
✅ Verification successful
Let me gather more information about the implementation and test coverage to make a thorough verification.
The centralized FormatNodeJSON helper is well-implemented and thoroughly tested
The helper function maintains robust behavior with:
- Proper nil and empty slice handling
- Structured JSON output with name, type, ID, and optional metadata
- Comprehensive error handling for invalid metadata
- 100% test coverage with various test cases
- Consistent usage across multiple commands (globsearch, leaderboard/keys)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the behavior of FormatNodeJSON helper
# Expected: Should find the helper implementation and its test coverage
# Find the helper implementation
echo "Searching for FormatNodeJSON implementation:"
rg -A 10 "func FormatNodeJSON" cmd/helpers/
# Verify test coverage
echo "Searching for FormatNodeJSON tests:"
rg -A 5 "Test.*FormatNodeJSON" cmd/helpers/
Length of output: 1255
Script:
#!/bin/bash
# Get the full implementation of FormatNodeJSON
echo "Full FormatNodeJSON implementation:"
cat cmd/helpers/formatter.go
echo -e "\nFull test implementation:"
cat cmd/helpers/formatter_test.go
# Check if there are any other files using this function
echo -e "\nUsages of FormatNodeJSON:"
rg "FormatNodeJSON" --type go
Length of output: 4584
cmd/helpers/formatter_test.go (2)
1-10: LGTM! Package and imports are well organized.
The package declaration and imports are appropriate for the test file's requirements.
12-120: Verify test coverage aligns with actual usage patterns.
Since this formatter is used by other components (like globsearch.go), let's verify that the test cases cover all usage patterns from the consuming code.
✅ Verification successful
Let me gather more information about the test coverage by examining the test cases in getMetadata_test.go since it appears to have additional test cases.
Test coverage is comprehensive and aligns with usage patterns
The test coverage in formatter_test.go adequately covers all usage patterns found in the codebase:
- The test cases cover all error conditions (nil nodes, empty nodes, invalid metadata)
- All fields are tested (Name, Type, ID, Metadata)
- Complex metadata scenarios are verified (valid JSON, empty JSON, nil metadata)
- The actual usage in
globsearch.goandkeys.goonly callsFormatNodeJSONwith valid node arrays, which is well-covered - The implementation in
formatter.gohas no edge cases that aren't tested
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all usages of FormatNodeJSON to ensure test cases cover all scenarios
rg -A 3 "FormatNodeJSON\(" --type go
# Look for any custom Node struct initialization patterns that should be covered in tests
ast-grep --pattern 'v1.Node{$$$}'
Length of output: 1567
Script:
#!/bin/bash
# Check the test cases in getMetadata_test.go
cat cmd/query/getMetadata/getMetadata_test.go
# Also check the actual implementation of FormatNodeJSON
cat cmd/helpers/formatter.go
Length of output: 9487
cmd/leaderboard/keys/keys.go (2)
1-16: LGTM! Package structure and imports are well organized.
The imports cover all necessary functionality for HTTP client, CLI commands, protobuf, and table rendering.
77-102: LGTM! Command initialization follows best practices.
The command setup is well-structured with clear documentation and proper initialization of options and flags.
cmd/leaderboard/keys/keys_test.go (1)
105-171: LGTM! Comprehensive nil condition handling
The test cases thoroughly cover all potential nil scenarios with proper error message validation.
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅ |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
cmd/query/getMetadata/getMetadata.go (1)
Line range hint
39-52: Consider enhancing command usability and validation.The command structure is solid, but could be improved with:
- Add validation for the output format flag
- Include examples in the command usage
func New() *cobra.Command { o := &options{} cmd := &cobra.Command{ Use: "get-metadata [node name]", Short: "Outputs the node with its metadata", + Example: ` # Get node metadata in JSON format + minefield get-metadata mynode + + # Get node metadata in table format + minefield get-metadata mynode --output table + + # Save node metadata to a file + minefield get-metadata mynode --output-file output.json`, Args: cobra.ExactArgs(1), RunE: o.Run, DisableAutoGenTag: true, } o.AddFlags(cmd) + + // Add validation for output format + cmd.PreRunE = func(cmd *cobra.Command, args []string) error { + if o.output != outputFormatJSON && o.output != outputFormatTable { + return fmt.Errorf("invalid output format %q: must be one of [json, table]", o.output) + } + return nil + } + return cmd }cmd/leaderboard/keys/keys_test.go (5)
20-103: Enhance test coverage with additional edge cases.Consider adding the following test cases to
TestRenderTable:
- Empty nodes array
- Very long node names/types that might affect table formatting
- Invalid output format
- Maximum number of columns with showInfo
173-201: Consider enhancing command validation.The test could be more comprehensive by:
- Validating default values for flags
- Testing command aliases if any
- Verifying short descriptions
- Testing flag validation rules
266-277: Consider enhancing type conversion handling.The helper function could be improved by:
- Adding nil value handling
- Supporting more numeric types (float32, float64, int64, etc.)
- Adding error handling for invalid conversions
298-377: Add test cases for different output configurations.Consider adding test cases for:
- JSON output format
- Different maxOutput values
- showInfo flag variations
- Different address configurations
- Command flag combinations
1-377: Consider centralizing test data management.The test file could benefit from having centralized test data fixtures to:
- Ensure consistency across test cases
- Make test data maintenance easier
- Reduce duplication of node definitions
- Make it easier to add new test scenarios
Consider creating shared test fixtures or helper functions for common test data setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
cmd/leaderboard/keys/keys_test.go(1 hunks)cmd/query/getMetadata/getMetadata.go(2 hunks)cmd/query/getMetadata/getMetadata_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- cmd/query/getMetadata/getMetadata_test.go
🔇 Additional comments (5)
cmd/query/getMetadata/getMetadata.go (2)
12-12: LGTM! Import changes align with the refactoring.
The addition of the helpers package and removal of encoding/json reflects the centralization of JSON formatting logic.
122-122: Verify the helper implementation for consistent behavior.
The refactoring improves code maintainability by centralizing JSON formatting logic. However, let's verify the helper implementation to ensure consistent behavior.
✅ Verification successful
Helper implementation verified and working as expected
The refactored FormatNodeJSON helper maintains consistent behavior with the original implementation while adding support for multiple nodes:
- Proper error handling for nil/empty inputs
- Consistent metadata handling and JSON formatting
- Well-tested with 100% coverage including edge cases
- Maintains the same output format and field structure
The change from single node to slice handling is implemented efficiently with pre-allocated capacity, and the conversion []*apiv1.Node{node} in the calling code is a negligible overhead.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the FormatNodeJSON helper implementation and its usage
# Check the helper implementation
echo "Checking FormatNodeJSON implementation:"
ast-grep --pattern 'func FormatNodeJSON($params) $ret {
$$$
}'
# Check for other usages of FormatNodeJSON
echo "Checking other usages of FormatNodeJSON:"
rg "FormatNodeJSON" --type go
# Check for any existing tests
echo "Checking for related tests:"
fd -e go -x rg -l "TestFormatNodeJSON|FormatNodeJSON.*testing"
Length of output: 1125
Script:
#!/bin/bash
# Let's examine the implementation and test coverage
# Check the implementation details
echo "=== FormatNodeJSON Implementation ==="
rg -A 10 "^func FormatNodeJSON" cmd/helpers/formatter.go
# Check the test cases
echo -e "\n=== Test Cases ==="
rg -A 20 "^func TestFormatNodeJSON" cmd/helpers/formatter_test.go
# Check the original implementation for comparison
echo -e "\n=== Original Implementation (if exists) ==="
git log -p | rg -A 10 -B 2 "func.*formatNodeJSON"
Length of output: 2172
Script:
#!/bin/bash
# Let's see the complete implementation and test coverage
# Get the complete implementation including the nodeOutput type
echo "=== Complete Implementation ==="
cat cmd/helpers/formatter.go
# Get the complete test file to see all test cases
echo -e "\n=== Complete Test File ==="
cat cmd/helpers/formatter_test.go
Length of output: 3813
cmd/leaderboard/keys/keys_test.go (3)
105-171: LGTM! Comprehensive nil checks.
The test thoroughly covers nil conditions with clear error messages and well-structured test cases.
203-264: LGTM! Well-structured flag testing.
The test thoroughly validates all flag properties using a clear table-driven approach.
279-296: LGTM! Clean mock implementation.
The mock client follows good practices with flexible behavior configuration.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores