migrate: align shared dependencies#183
Conversation
|
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. WalkthroughAdds a new dependency migration utility and test hook (ExecCommand); exports MigrateDependencies which downloads Fiber’s go.mod for a target release, extracts required dependency versions, scans local module go.mod files under cwd, bumps any lower versions to Fiber’s targets, writes updated go.mod files, and logs when changes occur. Wires this into the migration list and adds tests and stubs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as cobra.Command
participant DoMig as DoMigration
participant Deps as MigrateDependencies
participant Go as ExecCommand ("go mod download -json")
participant FS as Filesystem
User->>CLI: run migrate
CLI->>DoMig: DoMigration()
DoMig->>Deps: call MigrateDependencies(target)
Deps->>Go: exec "go mod download -json" for fiber@target
Go-->>Deps: JSON containing GoMod path
Deps->>FS: read Fiber go.mod (GoMod path)
Deps->>FS: discover module dirs (fiberModuleDirs)
loop for each module go.mod
Deps->>FS: read & parse module go.mod
Deps->>Deps: compare each require vs Fiber requirements
alt current < fiber requirement
Deps->>FS: update require, format, write go.mod
Deps-->>CLI: log "Updating dependency versions"
else no change
Deps-->>Deps: continue
end
end
Deps-->>DoMig: return error/nil
DoMig-->>CLI: aggregate results
CLI-->>User: output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
the
🤖 Dependencies
![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 new capability to automatically align project dependencies with the versions required by the Fiber framework. It adds a dedicated migration function that inspects and updates go.mod files to ensure compatibility. This change streamlines the upgrade process for projects using Fiber, reducing manual dependency management efforts. The new functionality is thoroughly tested and integrated into the existing migration command.
Highlights
- New Dependency Migration Function: A new
MigrateDependenciesfunction has been introduced. This function automates the process of aligning project dependencies with those required by a target Fiber framework release. It identifies shared dependencies and bumps their versions in the project'sgo.modfile if they are older than Fiber's requirements. - Integration into Migration Workflow: The newly implemented
MigrateDependenciesfunction is now integrated into the existing migration framework. This means that when a project undergoes a migration, its shared dependencies will automatically be checked and updated to ensure compatibility with the target Fiber version. - Enhanced Test Coverage and Infrastructure: Comprehensive unit tests have been added for the
MigrateDependenciesfunction, covering various scenarios including cases where dependencies need updating and where no changes are required. Additionally, the testing setup for themigratecommand has been enhanced to properly mock external command executions, ensuring reliable and isolated tests.
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. ↩
ReneWerner87
removed
the
🤖 Dependencies
There was a problem hiding this comment.
Code Review
This pull request introduces a new migration step to align shared dependencies with the versions required by the target Fiber release. The implementation is solid and well-tested. I've provided a few suggestions to improve error handling in command execution, enhance test robustness by using defer for cleanup in TestMain, and a note on test parallelization for future consideration.
| orig := migrations.ExecCommand | ||
|
|
||
| tmpDir, err := os.MkdirTemp("", "fiber_mod") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| fiberMod := `module github.com/gofiber/fiber/v2 | ||
|
|
||
| go 1.22 | ||
|
|
||
| require github.com/valyala/fasthttp v1.0.0` | ||
| fiberGoMod := filepath.Join(tmpDir, "go.mod") | ||
| if err := os.WriteFile(fiberGoMod, []byte(fiberMod), 0o600); err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| migrations.ExecCommand = func(string, ...string) *exec.Cmd { | ||
| return exec.Command("echo", fmt.Sprintf(`{"GoMod":%q}`, fiberGoMod)) // #nosec G204 -- testing stub | ||
| } | ||
|
|
||
| code := m.Run() | ||
|
|
||
| migrations.ExecCommand = orig | ||
| if err := os.RemoveAll(tmpDir); err != nil { | ||
| panic(err) | ||
| } | ||
| os.Exit(code) |
There was a problem hiding this comment.
Using defer to restore the original migrations.ExecCommand is safer. If m.Run() panics, the current implementation will not restore the original function, which could affect other tests running in the same process. The cleanup of the temporary directory should also be in a defer block to ensure it runs.
orig := migrations.ExecCommand
defer func() { migrations.ExecCommand = orig }()
tmpDir, err := os.MkdirTemp("", "fiber_mod")
if err != nil {
panic(err)
}
defer func() {
if err := os.RemoveAll(tmpDir); err != nil {
panic(err)
}
}()
fiberMod := `module github.com/gofiber/fiber/v2
go 1.22
require github.com/valyala/fasthttp v1.0.0`
fiberGoMod := filepath.Join(tmpDir, "go.mod")
if err := os.WriteFile(fiberGoMod, []byte(fiberMod), 0o600); err != nil {
panic(err)
}
migrations.ExecCommand = func(string, ...string) *exec.Cmd {
return exec.Command("echo", fmt.Sprintf(`{"GoMod":%q}`, fiberGoMod)) // #nosec G204 -- testing stub
}
os.Exit(m.Run())| var out bytes.Buffer | ||
| c.Stdout = &out | ||
| c.Stderr = &out | ||
| if err := c.Run(); err != nil { | ||
| return fmt.Errorf("download fiber module: %w", err) | ||
| } |
There was a problem hiding this comment.
Capturing stdout and stderr into the same buffer can lead to issues if the command writes to both streams. For instance, if go mod download writes an error or warning to stderr while also writing JSON to stdout, the combined output would be invalid JSON, causing json.Unmarshal to fail. It's safer to capture stderr in a separate buffer to provide clearer error messages.
| var out bytes.Buffer | |
| c.Stdout = &out | |
| c.Stderr = &out | |
| if err := c.Run(); err != nil { | |
| return fmt.Errorf("download fiber module: %w", err) | |
| } | |
| var out, stderr bytes.Buffer | |
| c.Stdout = &out | |
| c.Stderr = &stderr | |
| if err := c.Run(); err != nil { | |
| return fmt.Errorf("download fiber module: %w\n%s", err, stderr.String()) | |
| } |
| } | ||
|
|
||
| func Test_DoMigration_Verbose_Run(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
Removing t.Parallel() from this test and its sub-tests slows down the test suite. This was likely done to prevent race conditions from concurrent tests modifying the global migrations.ExecCommand. A more robust solution would be to refactor the code to avoid a package-level variable for dependency injection. For example, you could pass the exec.Command function as an argument or make it a field on a struct. This would allow tests to run in parallel without interfering with each other.
![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 (6)
cmd/internal/migrations/dependencies.go (2)
29-41: Avoid mixing stderr into the JSON buffer; capture and surface stderr on failure.
go mod download -jsonwrites machine-readable JSON to stdout. Piping stderr into the same buffer risks corrupting the JSON when the Go tool emits warnings on stderr. Capture stdout and stderr separately and include stderr in the error for easier diagnostics.Apply this diff:
- c := ExecCommand("go", "mod", "download", "-json", fiberModule) - var out bytes.Buffer - c.Stdout = &out - c.Stderr = &out - if err := c.Run(); err != nil { - return fmt.Errorf("download fiber module: %w", err) - } + c := ExecCommand("go", "mod", "download", "-json", fiberModule) + var stdout, stderr bytes.Buffer + c.Stdout = &stdout + c.Stderr = &stderr + if err := c.Run(); err != nil { + return fmt.Errorf("download fiber module: %w (stderr: %s)", err, strings.TrimSpace(stderr.String())) + } var info struct { GoMod string `json:"GoMod"` //nolint:tagliatelle // field name defined by go tool output } - if err := json.Unmarshal(out.Bytes(), &info); err != nil { + if err := json.Unmarshal(stdout.Bytes(), &info); err != nil { return fmt.Errorf("parse download info: %w", err) }
94-104: Minor: file mode for go.mod writes.You’re writing
go.modwith mode0o600. Typically,go.modis world-readable (0644). Not a blocker, but consider aligning with common defaults to avoid needless file mode diffs on some systems.Example change:
- if err := os.WriteFile(modFile, formatted, 0o600); err != nil { + if err := os.WriteFile(modFile, formatted, 0o644); err != nil {cmd/migrate_test.go (1)
43-45: Make the stub cross-platform (avoid relying on externalecho).Using
exec.Command("echo", ...)can be brittle on non-Unix environments. Prefer a helper process pattern that prints the JSON from within the Go test binary.Apply this diff:
- migrations.ExecCommand = func(string, ...string) *exec.Cmd { - return exec.Command("echo", fmt.Sprintf(`{"GoMod":%q}`, fiberGoMod)) // #nosec G204 -- testing stub - } + migrations.ExecCommand = func(string, ...string) *exec.Cmd { + cmd := exec.Command(os.Args[0], "-test.run=TestPrintGoModJSON", "--") + cmd.Env = append(os.Environ(), "GO_WANT_GOMOD_JSON=1", "GOMOD_PATH="+fiberGoMod) + return cmd + }Add this helper to the same test file:
// TestPrintGoModJSON is a helper invoked via -test.run func TestPrintGoModJSON(t *testing.T) { if os.Getenv("GO_WANT_GOMOD_JSON") != "1" { return } fmt.Printf(`{"GoMod":%q}`, os.Getenv("GOMOD_PATH")) os.Exit(0) }cmd/internal/migrations/lists_test.go (2)
20-21: Parallelization note.Top-level
t.Parallel()is fine here since this subtest suite doesn’t mutatemigrations.ExecCommand. Keep an eye on future tests in the same package that patch the global to avoid intermittent flakes.
79-105: Change-path coverage is correct; consider asserting dependency-migration status as well.You validate package import updates. Optionally, assert the verbose footer for
MigrateDependencies(“no changes”) to exercise the new per-function status output end-to-end.Example:
assert.Contains(t, out, "MigrateDependencies: no changes")cmd/internal/migrations/dependencies_test.go (1)
62-105: Nice no-op path coverage; add a pseudo-version case for robustness.Consider adding a case where Fiber’s required version is a pseudo-version (e.g.,
v1.10.0-0.20200102030405-abcdef123456) to ensure semver parsing and comparison behave as intended.Happy to draft that test if you’d like.
📜 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 (5)
cmd/internal/migrations/dependencies.go(1 hunks)cmd/internal/migrations/dependencies_test.go(1 hunks)cmd/internal/migrations/lists.go(1 hunks)cmd/internal/migrations/lists_test.go(3 hunks)cmd/migrate_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/lists_test.go
🧬 Code graph analysis (4)
cmd/internal/migrations/lists.go (2)
cmd/internal/migrations/common.go (1)
MigrateGoPkgs(21-58)cmd/internal/migrations/dependencies.go (1)
MigrateDependencies(26-111)
cmd/internal/migrations/lists_test.go (1)
cmd/internal/migrations/dependencies.go (1)
ExecCommand(18-18)
cmd/migrate_test.go (2)
cmd/internal/migrations/dependencies.go (1)
ExecCommand(18-18)cmd/internal/go_mod.go (1)
ExecCommand(15-15)
cmd/internal/migrations/dependencies_test.go (1)
cmd/internal/migrations/dependencies.go (2)
ExecCommand(18-18)MigrateDependencies(26-111)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build (1.25.x, windows-latest)
🔇 Additional comments (8)
cmd/internal/migrations/dependencies.go (4)
20-26: Clear migration intent and scope.Docstring and signature look good. Using the target’s major to resolve the Fiber module is the right approach for mapping requirements.
52-60: Semver parsing from go.mod is fine.Trimming the
vprefix and using Masterminds/semver is appropriate for Go module versions, including pre-releases. Good defensive error wrapping.
78-92: No downgrades; only bump when below Fiber’s minimum.The comparison/update logic is correct and won’t downgrade dependencies that already exceed Fiber’s floor. Nice.
107-110: Good UX signal.Printing “Updating dependency versions” only when changes occur keeps verbose output tidy and lets the caller’s wrapper decide “changed/no changes.” Works well with the lists.go verbose wrapper.
cmd/migrate_test.go (1)
26-54: Solid global test harness for fiber go.mod resolution.Patching
migrations.ExecCommandinTestMainto return a syntheticGoModJSON keeps tests hermetic. Cleanup and restoration are handled correctly.cmd/internal/migrations/lists.go (1)
33-33: Good call: run dependency floor alignment early.Adding
MigrateDependenciesalongsideMigrateGoPkgsin the first window ensures require-lines are bumped before later migrations andgo modoperations. Sensible sequencing.cmd/internal/migrations/lists_test.go (1)
52-67: Hermetic stubbing for “no changes” path looks good.Stubbing
migrations.ExecCommandto return a v2 fiber go.mod and asserting “MigrateGoPkgs: no changes”/skip messages is appropriate coverage.cmd/internal/migrations/dependencies_test.go (1)
18-61: Covers bump-only behavior without downgrades.Great check that
fasthttpis bumped to the floor whilebrotlistays at a higher local version. The log assertion ensures the changed-path signal is emitted.
github-actions
Bot
added
the
🤖 Dependencies
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
cmd/internal/migrations/exec_stub_test.go (4)
19-27: Don’t clobber process environment; also anchor -test.run to avoid accidental matchesOverwriting
cmd.Envdrops PATH and other env vars, which can be brittle in CI. Also,-test.run=TestHelperProcessis a regex and may match unintended tests in this package. Anchor it.Apply:
- migrations.ExecCommand = func(string, ...string) *exec.Cmd { - cmd := exec.Command(os.Args[0], "-test.run=TestHelperProcess", "--") // #nosec G204 -- test helper - cmd.Env = []string{ - "GO_WANT_HELPER_PROCESS=1", - "GO_HELPER_STDOUT=" + out, - } - return cmd - } + migrations.ExecCommand = func(string, ...string) *exec.Cmd { + cmd := exec.Command(os.Args[0], "-test.run=^TestHelperProcess$", "--") // #nosec G204 -- test helper + cmd.Env = append(os.Environ(), + "GO_WANT_HELPER_PROCESS=1", + "GO_HELPER_STDOUT="+out, + ) + return cmd + }
28-29: Auto-restore the stub with t.Cleanup to prevent leaks across testsRelying on callers to
defer restore()is easy to forget. Register a cleanup as well.- return func() { migrations.ExecCommand = orig } + t.Cleanup(func() { migrations.ExecCommand = orig }) + return func() { migrations.ExecCommand = orig }
19-19: Prefer json.Marshal over fmt.Sprintf for JSON constructionWhile
%qescapes correctly,encoding/jsonavoids edge cases and is clearer intent.-import ( - "fmt" +import ( + "encoding/json" + "fmt" "os" "os/exec" "path/filepath" "testing" ) @@ - out := fmt.Sprintf(`{"GoMod":%q}`, filepath.ToSlash(fiberGoMod)) + p := struct { + GoMod string `json:"GoMod"` + }{GoMod: filepath.ToSlash(fiberGoMod)} + b, err := json.Marshal(p) + if err != nil { + t.Fatalf("failed to marshal stub payload: %v", err) + } + out := string(b)
31-40: Support error-path simulation to increase test coverageMirroring the shared helper in cmd/tester_test.go lets you simulate failures from the stubbed command as well.
func TestHelperProcess(t *testing.T) { t.Helper() if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" { return } + if os.Getenv("GO_WANT_HELPER_NEED_ERR") == "1" { + _, _ = fmt.Fprint(os.Stderr, "fake error") + os.Exit(1) + } if out := os.Getenv("GO_HELPER_STDOUT"); out != "" { _, _ = fmt.Fprint(os.Stdout, out) } os.Exit(0) //nolint:revive // helper process exits intentionally }Optional: If duplication with other packages becomes a maintenance burden, consider moving a generic helper into a shared internal testutil package.
cmd/tester_test.go (1)
60-62: Optional: consider newline semantics for CLI-like output.Many CLIs end lines with a newline. If that’s desired, switch to Fprintln; otherwise, keep as-is and include “\n” in GO_HELPER_STDOUT in tests.
- _, _ = fmt.Fprint(os.Stdout, out) + _, _ = fmt.Fprintln(os.Stdout, out)
📜 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 (5)
cmd/internal/migrations/dependencies_test.go(1 hunks)cmd/internal/migrations/exec_stub_test.go(1 hunks)cmd/internal/migrations/lists_test.go(2 hunks)cmd/migrate_test.go(3 hunks)cmd/tester_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/migrate_test.go
- cmd/internal/migrations/dependencies_test.go
- cmd/internal/migrations/lists_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/exec_stub_test.go (1)
cmd/tester_test.go (1)
TestHelperProcess(35-65)
🔇 Additional comments (2)
cmd/internal/migrations/exec_stub_test.go (1)
1-11: Good use of external test package and helper-process patternBlack-boxing via
migrations_testand stubbing through an exportedExecCommandkeeps tests realistic while controllable. Imports are minimal and all used.cmd/tester_test.go (1)
60-62: The script to inspectcmd/tester_test.gohas been queued—will review the output to confirm howTestHelperProcesshandlesGO_HELPER_STDOUTand validate preserving the parent environment is necessary.
| func stubFiberDownload(t *testing.T, fiberGoMod string) func() { | ||
| t.Helper() | ||
| orig := migrations.ExecCommand | ||
| out := fmt.Sprintf(`{"GoMod":%q}`, filepath.ToSlash(fiberGoMod)) | ||
| migrations.ExecCommand = func(string, ...string) *exec.Cmd { | ||
| cmd := exec.Command(os.Args[0], "-test.run=TestHelperProcess", "--") // #nosec G204 -- test helper | ||
| cmd.Env = []string{ | ||
| "GO_WANT_HELPER_PROCESS=1", | ||
| "GO_HELPER_STDOUT=" + out, | ||
| } | ||
| return cmd | ||
| } | ||
| return func() { migrations.ExecCommand = orig } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Global override is not parallel-safe
Because migrations.ExecCommand is a global, parallel tests that also mutate it can race. Either guard tests that use this stub from running in parallel or serialize access with a package-level test mutex.
You can quickly check if any tests in this package use t.Parallel():
🏁 Script executed:
#!/bin/bash
# Search for parallel tests and stub usage within the migrations tests
rg -nP --type=go -C1 '\bt\.Parallel\(\)' cmd/internal/migrations
rg -nP --type=go -C2 '\bstubFiberDownload\s*\(' cmd/internal/migrationsLength of output: 20720
Protect migrations.ExecCommand Stub from Parallel Execution
The stubFiberDownload helper globally overrides migrations.ExecCommand, but it’s invoked inside parallel tests in lists_test.go, causing a data race when multiple subtests run concurrently. To resolve this, either serialize access to the global override or disable parallelism for those subtests.
• In cmd/internal/migrations/lists_test.go
– Subtest "silent" (t.Run at line 23) calls t.Parallel() then invokes stubFiberDownload (line 60).
– Subtest "verbose" (t.Run at line 33) calls t.Parallel() then invokes stubFiberDownload (line 83).
Options:
- Remove the
t.Parallel()calls in those subtests, or - Introduce a package-level
sync.Mutexto guard the assignment/restoration instubFiberDownload.
• In cmd/internal/migrations/dependencies_test.go
– stubFiberDownload is used (line 43) but the surrounding test isn’t marked parallel—no immediate conflict, though future parallelization could expose the same issue.
Addressing this will ensure test isolation and eliminate flakiness from concurrent overrides.
🤖 Prompt for AI Agents
In cmd/internal/migrations/exec_stub_test.go around lines 16-29, the
stubFiberDownload helper globally overrides migrations.ExecCommand causing a
race when tests run in parallel; fix by adding a package-level sync.Mutex (e.g.,
var execCommandMu sync.Mutex) and wrap the assignment/restoration inside
stubFiberDownload with execCommandMu.Lock() before setting
migrations.ExecCommand and ensure execCommandMu.Unlock() happens in the returned
cleanup function (use defer-style unlock inside that cleanup), and add the sync
import; alternatively, remove t.Parallel() from the conflicting subtests in
cmd/internal/migrations/lists_test.go (the "silent" and "verbose" subtests) so
the global override is not used concurrently.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/migrate_test.go (1)
26-59: Harden TestMain: ensure cleanup/restoration even if future changes panic; also preserve parent environment.
- Using a closure around m.Run() lets you use defer to reliably restore migrations.ExecCommand and delete tmpDir before os.Exit executes.
- Don’t clobber the entire environment for the helper process; append to os.Environ() to avoid brittle behavior on some systems and CI.
Apply this diff within TestMain:
func TestMain(m *testing.M) { - orig := migrations.ExecCommand - - tmpDir, err := os.MkdirTemp("", "fiber_mod") - if err != nil { - panic(err) - } - fiberMod := `module github.com/gofiber/fiber/v2 - -go 1.22 - -require github.com/valyala/fasthttp v1.0.0` - fiberGoMod := filepath.Join(tmpDir, "go.mod") - if err := os.WriteFile(fiberGoMod, []byte(fiberMod), 0o600); err != nil { - panic(err) - } - - migrations.ExecCommand = func(string, ...string) *exec.Cmd { - cmd := exec.Command(os.Args[0], "-test.run=TestHelperProcess", "--", "go", "mod", "download") // #nosec G204 -- test helper - cmd.Env = []string{ - "GO_WANT_HELPER_PROCESS=1", - "GO_HELPER_STDOUT=" + fmt.Sprintf(`{"GoMod":%q}`, filepath.ToSlash(fiberGoMod)), - } - return cmd - } - - code := m.Run() - - migrations.ExecCommand = orig - if err := os.RemoveAll(tmpDir); err != nil { - panic(err) - } - os.Exit(code) + code := func() int { + orig := migrations.ExecCommand + defer func() { migrations.ExecCommand = orig }() + + tmpDir, err := os.MkdirTemp("", "fiber_mod") + if err != nil { + panic(err) + } + defer func() { + if err := os.RemoveAll(tmpDir); err != nil { + panic(err) + } + }() + + fiberMod := `module github.com/gofiber/fiber/v2 + +go 1.22 + +require github.com/valyala/fasthttp v1.0.0` + fiberGoMod := filepath.Join(tmpDir, "go.mod") + if err := os.WriteFile(fiberGoMod, []byte(fiberMod), 0o600); err != nil { + panic(err) + } + + migrations.ExecCommand = func(string, ...string) *exec.Cmd { + cmd := exec.Command(os.Args[0], "-test.run=TestHelperProcess", "--", "go", "mod", "download") // #nosec G204 -- test helper + cmd.Env = append(os.Environ(), + "GO_WANT_HELPER_PROCESS=1", + "GO_HELPER_STDOUT="+fmt.Sprintf(`{"GoMod":%q}`, filepath.ToSlash(fiberGoMod)), + ) + return cmd + } + return m.Run() + }() + os.Exit(code) }
🧹 Nitpick comments (1)
cmd/migrate_test.go (1)
26-59: Optional: add an explicit dependency-alignment assertion.You already seed Fiber’s go.mod with fasthttp v1.0.0 via TestMain. To directly cover the “align shared dependencies” migration, consider a focused test that starts with a local go.mod requiring an older fasthttp (e.g., v0.0.x), runs migrate, and asserts the local go.mod is bumped to v1.0.0. This will validate the new generic dependency migration end-to-end.
Happy to draft that test if you want it in this file.
Also applies to: 65-66
📜 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 (3)
cmd/internal/migrations/lists.go(1 hunks)cmd/internal/migrations/lists_test.go(2 hunks)cmd/migrate_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/internal/migrations/lists.go
- cmd/internal/migrations/lists_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-23T19:50:41.765Z
Learnt from: norri
PR: gofiber/recipes#2701
File: clean-code/app/server/server.go:14-16
Timestamp: 2024-11-23T19:50:41.765Z
Learning: In the `clean-code` example at `clean-code/app/server/server.go` using the Go Fiber framework, it's acceptable to omit production-level features like context usage and graceful shutdown handling to keep the example simple.
Applied to files:
cmd/migrate_test.go
📚 Learning: 2024-11-23T19:35:36.767Z
Learnt from: norri
PR: gofiber/recipes#2701
File: clean-code/app/main.go:0-0
Timestamp: 2024-11-23T19:35:36.767Z
Learning: In the Go `clean-code` example (`clean-code/app/main.go`) in the `gofiber/recipes` repository, it's acceptable to omit graceful shutdown handling, as the example code prioritizes simplicity over production-level practices.
Applied to files:
cmd/migrate_test.go
🧬 Code graph analysis (1)
cmd/migrate_test.go (2)
cmd/internal/migrations/dependencies.go (1)
ExecCommand(18-18)cmd/internal/go_mod.go (1)
ExecCommand(15-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, ubuntu-latest)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, windows-latest)
🔇 Additional comments (3)
cmd/migrate_test.go (3)
4-4: Import addition is appropriate.fmt is used for JSON formatting in TestMain; no issues.
16-16: Correct internal import for stubbing.Importing cmd/internal/migrations to patch ExecCommand is the right seam for these tests.
65-66: Mixed baseline version in migration tests — please verify intent
The test assertions for migration messages currently use two different “from” versions:
- cmd/migrate_test.go: lines 146 & 189 expect “Migration from Fiber 2.0.6 to 3.0.0”
- cmd/migrate_test.go: lines 154, 280, 306 & 338 expect “Migration from Fiber 2.0.0 to 3.0.0”
Since go.mod now pins fiber/v2 at v2.0.6, please confirm whether the 2.0.0 expectations (e.g., in forced‐run tests) are intentional. If they aren’t, update those assertions to match 2.0.6 (or better yet, derive the “from” version from a shared goModV2 constant) to keep tests DRY and avoid brittleness when the baseline version changes.
Affected locations:
- cmd/migrate_test.go:154, 280, 306, 338
1 participant
Summary
Testing
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_68aa3b2f6e20832697d01e43ac24e33c
Summary by CodeRabbit
New Features
Tests