🧹 chore: Improve hooks test coverage #3524
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughMultiple new test functions have been added to Changes
Suggested reviewers
Poem
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. 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 (
|
![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 @gaby, 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 significantly improves the robustness of the application's hook system by expanding its test coverage. The primary focus is on validating how various hook execution functions behave when a registered hook returns an error, ensuring that such scenarios are gracefully handled and reported. It also confirms correct path resolution for mounted applications within the hook context.
Highlights
- Enhanced Test Coverage: Added a suite of new unit tests to
hooks_test.goto specifically increase test coverage for error handling paths within the application's hook execution functions. - Error Scenario Validation: Introduced dedicated tests for
executeOnRouteHooks,executeOnNameHooks,executeOnGroupHooks,executeOnGroupNameHooks,executeOnListenHooks,executeOnPreShutdownHooks,executeOnForkHooks, andexecuteOnMountHooksto ensure they correctly handle and propagate errors returned by registered hooks. - Mounted Application Context Testing: New tests verify that the
mountPathis correctly prefixed toRouteandGrouppaths when hooks are executed within the context of a mounted application.
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 is currently in preview and 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 to provide feedback.
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. ↩
gaby
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3524 +/- ##
==========================================
+ Coverage 89.06% 89.28% +0.22%
==========================================
Files 111 111
Lines 11163 11163
==========================================
+ Hits 9942 9967 +25
+ Misses 920 903 -17
+ Partials 301 293 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
The code changes add new test cases to increase the test coverage for the hooks.go file, specifically focusing on error scenarios for various hook types when a mount path is involved. The new tests cover OnRoute, OnName, OnGroup, OnGroupName, OnListen, OnPreShutdown, OnFork, and OnMount hooks, ensuring that errors returned by these hooks are properly handled and asserted.
![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
🧹 Nitpick comments (1)
hooks_test.go (1)
350-400: Extract the mount path literal into a shared constant to silencegoconstand improve maintainabilityThe string
"/api"appears four times across the newly-added tests (lines 350, 366, 382 and 398).
golangci-lintis already flagging this duplication. Moving the literal to a package-level constant keeps the tests DRY and removes the lint failure.+const testMountPath = "/api" … - app.mountFields.mountPath = "/api" + app.mountFields.mountPath = testMountPathApply the same replacement in the other three occurrences.
This keeps the intent clear and prevents future copy-paste errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks_test.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
hooks_test.go
[failure] 350-350:
string /api has 4 occurrences, make it a constant (goconst)
🪛 GitHub Actions: golangci-lint
hooks_test.go
[error] 350-350: golangci-lint: string /api has 4 occurrences, make it a constant (goconst)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: repeated
- GitHub Check: Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 38d5e32 | Previous: 0590709 | Ratio |
|---|---|---|---|
Benchmark_Ctx_SendString_B |
16.17 ns/op 0 B/op 0 allocs/op |
9.049 ns/op 0 B/op 0 allocs/op |
1.79 |
Benchmark_Ctx_SendString_B - ns/op |
16.17 ns/op |
9.049 ns/op |
1.79 |
BenchmarkDecoderedirectionMsg - MB/s |
414.92 MB/s |
250.12 MB/s |
1.66 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
hooks_test.go (2)
448-451: Same global-logger race in fork-hook test
Issue & fix are identical to the previous comment – reuse the same pattern.
425-437: Parallel test mutates global logger – data race & flaky output
log.SetOutput(&buf)changes a global singleton while this test is flaggedt.Parallel().
Any other parallel test that writes to or re-configures the logger will race.- t.Parallel() - var buf bytes.Buffer - log.SetOutput(&buf) + // Cannot run in parallel because we mutate global logger state. + // t.Parallel() + + orig := log.Writer() // preserve + var buf bytes.Buffer + log.SetOutput(&buf) + defer log.SetOutput(orig)Either serialise the test (drop
t.Parallel()), or guardlog.SetOutputwith a shared mutex used by every test touching the logger.
🧹 Nitpick comments (1)
hooks_test.go (1)
4-10: Avoid confusing alias ‑ import Fiber logger under a distinct nameImporting
github.com/gofiber/fiber/v3/logas plainlogshadows the standard library’slogpackage and can surprise future readers/tools.
Consider a clearer alias such asfiberlog:- "github.com/gofiber/fiber/v3/log" + fiberlog "github.com/gofiber/fiber/v3/log"All subsequent references (
log.SetOutput, etc.) would then usefiberlog.*, making intent unambiguous.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: lint
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.24.x, ubuntu-latest)
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: Analyse
Summary