fix: add testify import for utils migration#167
Conversation
WalkthroughIntroduces addImport to inject Go imports and updates MigrateUtilsImport to auto-add github.com/stretchr/testify/assert when replacing utils.AssertEqual with assert.Equal. Corresponding tests are updated to reflect rewritten utils imports/usages and the new assert import and usage. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Mig as MigrateUtilsImport
participant AI as addImport
Dev->>Mig: Run migration on Go source content
activate Mig
Mig->>Mig: Rewrite utils.* usages<br/>Replace utils.AssertEqual → assert.Equal
alt "assert.Equal" detected
Mig->>AI: addImport(content, "github.com/stretchr/testify/assert")
activate AI
AI-->>Mig: content with assert import injected
deactivate AI
else No assert usage
Mig-->>Dev: content with utils rewrites only
end
Mig-->>Dev: Final migrated content
deactivate Mig
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
Bot
added
☢️ Bug
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a fix to the v3 migration script, specifically enhancing the MigrateUtilsImport function. The primary goal is to ensure that when utils.AssertEqual is migrated to assert.Equal, the necessary github.com/stretchr/testify/assert import is automatically added to the Go files. This change also includes new test cases to validate the correct handling of this import.
Highlights
- Automated Testify Import: Implemented a new
addImporthelper function to dynamically add Go package imports, ensuringgithub.com/stretchr/testify/assertis included whenutils.AssertEqualis replaced withassert.Equal. - Migration Logic Enhancement: Updated the
MigrateUtilsImportfunction to utilize the newaddImportlogic, making the migration ofutils.AssertEqualmore robust by handling its dependency. - Improved Migration Test Coverage: Extended the test suite for
utilsmigration to explicitly cover theutils.AssertEqualreplacement and verify the correct addition of thetestify/assertimport.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/internal/migrations/v3/common.go (1)
560-564: Add missing stdlib import for strings helpers introduced by this migration.You rewrite utils string helpers to strings.Trim*/strings.EqualFold but never ensure the "strings" package is imported. This will break compilation for files that didn’t already import strings.
Add this guard right after mapping utils.* → strings.*:
// map removed string helpers to the strings package content = strings.ReplaceAll(content, "utils.TrimRight", "strings.TrimRight") content = strings.ReplaceAll(content, "utils.TrimLeft", "strings.TrimLeft") content = strings.ReplaceAll(content, "utils.Trim", "strings.Trim") content = strings.ReplaceAll(content, "utils.EqualFold", "strings.EqualFold") + + // ensure stdlib strings import exists when we introduced strings.* calls + if strings.Contains(content, "strings.Trim") || strings.Contains(content, "strings.EqualFold") { + content = addImport(content, "strings") + }Consider extending tests to assert that the "strings" import is present after migration. I can add that if you want.
🧹 Nitpick comments (2)
cmd/internal/migrations/v3/common.go (1)
535-538: Avoid creating multiple import blocks when multiple single-line imports exist.The ReplaceAllString call will convert every single-line import into a separate import block, which can break files that have multiple single import declarations. Convert only the first single-line import into a block and insert the new import there.
Apply this diff:
-reSingle := regexp.MustCompile(`(?m)^import\s+([^\n]+)`) // matches single import line -if reSingle.MatchString(content) { - return reSingle.ReplaceAllString(content, fmt.Sprintf("import (\n\t$1\n\t%s\n)", importStmt)) -} +reSingle := regexp.MustCompile(`(?m)^import\s+([^\n]+)$`) // matches a single import declaration on its own line +if loc := reSingle.FindStringIndex(content); loc != nil { + sub := reSingle.FindStringSubmatch(content[loc[0]:loc[1]]) + if len(sub) == 2 { + repl := fmt.Sprintf("import (\n\t%s\n\t%s\n)", sub[1], importStmt) + return content[:loc[0]] + repl + content[loc[1]:] + } +}cmd/internal/migrations/v3/common_test.go (1)
587-588: Assertion for testify import is correct and necessary.This verifies the core objective of the PR (auto-import assert). Once you add the “strings” import guard in the migration, consider adding a sibling assertion to ensure "strings" is imported when strings.* calls are introduced.
I can update the test to also assert the presence of the strings import if you apply the migration change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cmd/internal/migrations/v3/common.go(2 hunks)cmd/internal/migrations/v3/common_test.go(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
cmd/internal/migrations/v3/common_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
cmd/internal/migrations/v3/common_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
cmd/internal/migrations/v3/common_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
cmd/internal/migrations/v3/common_test.go
🪛 GitHub Check: lint
cmd/internal/migrations/v3/common.go
[failure] 525-525:
sprintfQuotedString: use %q instead of "%s" for quoted strings (gocritic)
🪛 GitHub Actions: golangci-lint
cmd/internal/migrations/v3/common.go
[error] 525-525: golangci-lint (gocritic): sprintfQuotedString should use %q instead of "%s" for quoted strings.
🔇 Additional comments (4)
cmd/internal/migrations/v3/common.go (1)
576-578: Good: conditional assert import injection is idempotent and localized.Replacing utils.AssertEqual → assert.Equal and then calling addImport only when assert.Equal is present keeps the change tight and avoids duplicate imports.
cmd/internal/migrations/v3/common_test.go (3)
578-579: Nice trigger for the migration path.Adding a utils.AssertEqual usage in the fixture is a clean way to exercise the assert import injection.
597-597: Covers removal of legacy helper usage.Ensuring utils.AssertEqual no longer appears post-migration is the right negative check.
609-609: Validates new API usage.Checking for assert.Equal confirms the rewrite outcome.
| } | ||
|
|
||
| func addImport(content, pkg string) string { | ||
| importStmt := fmt.Sprintf("\"%s\"", pkg) |
There was a problem hiding this comment.
Fix lint: use %q instead of "%s" when building a quoted string.
golangci-lint/gocritic is right here. Use %q to build a quoted import literal.
Apply this diff:
- importStmt := fmt.Sprintf("\"%s\"", pkg)
+ importStmt := fmt.Sprintf("%q", pkg)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| importStmt := fmt.Sprintf("\"%s\"", pkg) | |
| importStmt := fmt.Sprintf("%q", pkg) |
🧰 Tools
🪛 GitHub Check: lint
[failure] 525-525:
sprintfQuotedString: use %q instead of "%s" for quoted strings (gocritic)
🪛 GitHub Actions: golangci-lint
[error] 525-525: golangci-lint (gocritic): sprintfQuotedString should use %q instead of "%s" for quoted strings.
🤖 Prompt for AI Agents
In cmd/internal/migrations/v3/common.go around line 525, the code builds a
quoted import string using fmt.Sprintf("\"%s\"", pkg); replace that with
fmt.Sprintf("%q", pkg) to use the correct formatting verb for quoted strings and
satisfy the linter; update the call accordingly so the resulting string remains
the same but uses %q.
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
1 participant
Summary
Testing
go mod tidygo mod downloadgo mod vendorgo test ./...https://chatgpt.com/codex/tasks/task_e_68a8f400419c8326b1b5850b6929eb99
Summary by CodeRabbit
New Features
Bug Fixes
Tests