Sanitize snapshots #1002
Sanitize snapshots #1002
Conversation
osterman
added
no-release
📝 WalkthroughWalkthroughThis pull request introduces changes to the test suite. In Changes
Suggested labels
Suggested reviewers
Possibly related PRs
✨ Finishing Touches
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/cli_test.go (2)
181-194: Consider caching the repository root path.The repository root is looked up for each sanitization call, which could be inefficient for large outputs with multiple replacements. Consider caching the result.
var ( regenerateSnapshots = flag.Bool("regenerate-snapshots", false, "Regenerate all golden snapshots") startingDir string snapshotBaseDir string + cachedRepoRoot string ) func sanitizeOutput(output string) (string, error) { + if cachedRepoRoot == "" { repoRoot, err := findGitRepoRoot(startingDir) if err != nil { return "", err } if repoRoot == "" { return "", errors.New("failed to determine repository root") } + cachedRepoRoot = repoRoot + } - return strings.ReplaceAll(output, repoRoot, "/absolute/path/to/repo"), nil + return strings.ReplaceAll(output, cachedRepoRoot, "/absolute/path/to/repo"), nil }
738-758: Enhance error messages with context.The error messages could be more descriptive to help with debugging.
func findGitRepoRoot(path string) (string, error) { repo, err := git.PlainOpenWithOptions(path, &git.PlainOpenOptions{DetectDotGit: true}) if err != nil { - return "", fmt.Errorf("failed to find git repository: %w", err) + return "", fmt.Errorf("failed to find git repository starting from %q: %w", path, err) } worktree, err := repo.Worktree() if err != nil { - return "", fmt.Errorf("failed to get worktree: %w", err) + return "", fmt.Errorf("failed to get worktree for repository at %q: %w", path, err) } root, err := filepath.Abs(worktree.Filesystem.Root()) if err != nil { - return "", fmt.Errorf("failed to get absolute path of repository root: %w", err) + return "", fmt.Errorf("failed to get absolute path of repository root from %q: %w", worktree.Filesystem.Root(), err) } return root, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/cli_test.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
tests/cli_test.go (2)
666-675: LGTM! Robust error handling for output sanitization.The implementation properly sanitizes both stdout and stderr outputs with appropriate error handling.
582-582: LGTM! Improved octal literal notation.The update to use
0oprefix for octal literals improves code clarity and follows modern Go conventions.Also applies to: 586-586
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/cli_test.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
tests/cli_test.go (4)
19-19: LGTM! Import added for Git repository handling.The addition of the go-git package is appropriate for implementing repository root detection.
181-201: Well-structured sanitization function!The function effectively normalizes and replaces repository paths while maintaining proper error handling.
673-682: LGTM! Robust output sanitization added.The addition of output sanitization with proper error handling improves the snapshot verification process.
589-589: LGTM! Updated to modern octal notation.The change from decimal to octal notation (e.g.,
0o755) follows Go's preferred syntax for file permissions.Also applies to: 593-593
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/cli_test.go (1)
744-765: Consider adding error context and documentation.While the implementation is correct, consider enhancing the function with:
- GoDoc comments explaining the function's purpose and parameters
- More descriptive error messages that include the input path
-// findGitRepo finds the Git repository root +// findGitRepoRoot returns the absolute path to the Git repository root directory, +// starting from the given path. It returns an error if the path is not within a +// Git repository or if any Git operations fail. func findGitRepoRoot(path string) (string, error) { // Open the Git repository starting from the given path repo, err := git.PlainOpenWithOptions(path, &git.PlainOpenOptions{DetectDotGit: true}) if err != nil { - return "", fmt.Errorf("failed to find git repository: %w", err) + return "", fmt.Errorf("failed to find git repository at %q: %w", path, err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/cli_test.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
tests/cli_test.go (3)
181-201: LGTM! Well-structured sanitization function.The
sanitizeOutputfunction effectively normalizes and sanitizes repository paths in test outputs, ensuring consistent snapshots across different environments.
589-589: LGTM! Proper octal notation.The update from decimal to octal notation (
0755→0o755and0644→0o644) follows Go's preferred syntax for octal literals.Also applies to: 593-593
673-682: LGTM! Robust error handling.The error handling for output sanitization is thorough and ensures test failures are properly reported when sanitization fails.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/cli_test.go (2)
181-217: Consider caching the repository root.The function looks good but repeatedly calls
findGitRepoRootfor each output. Consider caching the repository root since it won't change during test execution.+var cachedRepoRoot string + func sanitizeOutput(output string) (string, error) { - repoRoot, err := findGitRepoRoot(startingDir) - if err != nil { - return "", err - } + if cachedRepoRoot == "" { + root, err := findGitRepoRoot(startingDir) + if err != nil { + return "", err + } + cachedRepoRoot = root + } - if repoRoot == "" { + if cachedRepoRoot == "" { return "", errors.New("failed to determine repository root") } // Clean the repo root to collapse multiple slashes, then normalize it to use forward slashes. - cleanRepoRoot := filepath.Clean(repoRoot) + cleanRepoRoot := filepath.Clean(cachedRepoRoot)
684-699: Consider grouping error handling for sanitization.The error handling for sanitization could be more concise.
- var err error - stdoutOutput, err = sanitizeOutput(stdoutOutput) - if err != nil { - t.Fatalf("failed to sanitize stdout output: %v", err) - } - stderrOutput, err = sanitizeOutput(stderrOutput) - if err != nil { - t.Fatalf("failed to sanitize stderr output: %v", err) - } + sanitize := func(output string, name string) string { + sanitized, err := sanitizeOutput(output) + if err != nil { + t.Fatalf("failed to sanitize %s output: %v", name, err) + } + return sanitized + } + stdoutOutput = sanitize(stdoutOutput, "stdout") + stderrOutput = sanitize(stderrOutput, "stderr")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/cli_test.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
tests/cli_test.go (3)
760-781: LGTM! Well-structured Git repository root finder.The function is well-documented and handles errors appropriately.
604-613: LGTM! Proper octal literal usage.The update to use
0oprefix for octal literals improves code clarity.
19-19: Verify the go-git version for security considerations.The code adds a new dependency on go-git. Let's verify the version for any known vulnerabilities.
Also applies to: 30-34
✅ Verification successful
go-git dependency is at a secure version (v5) outside the vulnerable range
The advisory indicates that go-git versions between 4.0.0 and 4.13.1 are affected. Our import uses
github.com/go-git/go-git/v5, which corresponds to version 5 and is not covered by that vulnerability.
- File:
tests/cli_test.go— confirm thatgithub.com/go-git/go-git/v5is used.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories for go-git # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/go-git/go-git") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 587
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/cli_test.go (1)
181-222: Consider wrapping errors for better context.The function is well-implemented and documented. However, consider wrapping errors with additional context.
Apply this diff to improve error handling:
func sanitizeOutput(output string) (string, error) { repoRoot, err := findGitRepoRoot(startingDir) if err != nil { - return "", err + return "", fmt.Errorf("failed to find repository root: %w", err) } if repoRoot == "" { return "", errors.New("failed to determine repository root") } // ... rest of the function ... repoRootRegex, err := regexp.Compile(pattern) if err != nil { - return "", err + return "", fmt.Errorf("failed to compile repository root regex: %w", err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/cli_test.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
tests/cli_test.go (4)
765-786: LGTM! Well-structured Git repository root finder.The function has good error handling and uses the go-git library effectively.
609-618: LGTM! Proper use of octal literals.The update to use
0oprefix for octal literals is correct and follows Go conventions.
689-704: LGTM! Robust output sanitization.The addition of output sanitization in verifySnapshot is well-implemented with proper error handling.
19-19: LGTM! Clean variable declarations and imports.The new variables are well-organized in a var block, and the go-git import is appropriate for Git operations.
Let's verify the go-git version in go.mod:
Also applies to: 30-34
✅ Verification successful
Import version verified and test file changes look good.
- The
go.modfile shows the correct version ofgithub.com/go-git/go-git/v5(v5.13.2).- The import and new variable declarations in
tests/cli_test.goare consistent with the verified dependency version.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the go-git version in go.mod rg -A 1 "github.com/go-git/go-git/v5" go.modLength of output: 113
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/cli_test.go (2)
183-250: Enhance error handling in sanitizeOutput.The function is well-structured with clear steps and comments. However, the error handling could be more robust.
Consider these improvements:
func sanitizeOutput(output string) (string, error) { + if output == "" { + return output, nil // Early return for empty strings + } repoRoot, err := findGitRepoRoot(startingDir) if err != nil { return "", err } if repoRoot == "" { return "", errors.New("failed to determine repository root") } // ... rest of the function ... repoRootRegex, err := regexp.Compile(pattern) if err != nil { - return "", err + return "", fmt.Errorf("failed to compile repository root regex: %w", err) }
793-814: Add input validation to findGitRepoRoot.While the function is well-implemented, it should validate the input path.
Consider adding input validation:
func findGitRepoRoot(path string) (string, error) { + if path == "" { + return "", errors.New("path cannot be empty") + } + + // Ensure the path exists + if _, err := os.Stat(path); err != nil { + return "", fmt.Errorf("invalid path %q: %w", path, err) + } + // Open the Git repository starting from the given path repo, err := git.PlainOpenWithOptions(path, &git.PlainOpenOptions{DetectDotGit: true}) // ... rest of the function ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/cli_test.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
tests/cli_test.go (3)
19-19: LGTM! Clean import and variable declarations.The new import and variable declarations are well-structured and necessary for the Git repository functionality and snapshot management.
Also applies to: 30-34
722-731: LGTM! Robust snapshot sanitization.The snapshot verification now properly sanitizes outputs and handles errors appropriately.
638-638: LGTM! Proper use of octal notation.The update to use octal notation (
0o755and0o644) for file permissions follows Go's preferred format.Also applies to: 642-642
|
These changes were released in v1.159.0. |
* sanitize snapshots * [autofix.ci] apply automated fixes * check for empty repo root * normalize slashes * normalize slashes * try to fix windows snapshots with windows paths * handle multiple slashes * handle multiple slashes * changed strategy for removing double slashes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
* sanitize snapshots * [autofix.ci] apply automated fixes * check for empty repo root * normalize slashes * normalize slashes * try to fix windows snapshots with windows paths * handle multiple slashes * handle multiple slashes * changed strategy for removing double slashes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
what
why
references
Summary by CodeRabbit
New Features
Bug Fixes
Tests
These improvements contribute to a cleaner and more consistent configuration display, leading to a better overall user experience.