Skip to content

Conversation

@dgageot
Copy link
Member

@dgageot dgageot commented Jan 20, 2026

I tried to use a cagent based agent to modernize the Go unit tests as much as possible.

- Replace for i := 0; i < b.N; i++ with for b.Loop() (Go 1.24)
- Replace os.Setenv/defer os.Unsetenv with t.Setenv() (Go 1.18)

Assisted-By: cagent
Use t.TempDir() instead of manual os.MkdirTemp/defer os.RemoveAll patterns.
This is cleaner, automatically handles cleanup on test failure, and is the
recommended approach since Go 1.14.

Assisted-By: cagent
Use t.Context() instead of context.Background() in tests. This is cleaner
and ensures the context is properly tied to the test's lifecycle.
Available since Go 1.24.

Assisted-By: cagent
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @dgageot, 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 refactors the Go unit tests to align with modern best practices, primarily by integrating testing.T's built-in utilities for context, temporary directories, and environment variables. These changes, coupled with new linter rules, aim to enhance test isolation, simplify resource management, and improve the overall robustness and maintainability of the test suite.

Highlights

  • Linter Rule Enforcement: New forbidigo linter rules have been added to .golangci.yml to enforce modern Go testing practices. These rules specifically disallow direct usage of os.MkdirTemp(), os.Setenv(), context.Background(), and context.TODO() within test files, guiding developers to use their testing.T equivalents.
  • Test Context Adoption: All instances of context.Background() and context.TODO() within test functions have been replaced with t.Context(), ensuring that test contexts are properly managed and tied to the lifecycle of the test.
  • Temporary Directory Management: Direct calls to os.MkdirTemp() have been replaced with t.TempDir() across numerous test files. This change leverages Go's built-in temporary directory management, which automatically cleans up directories after tests complete, improving test reliability and reducing resource leaks.
  • Environment Variable Handling in Tests: Usage of os.Setenv() and os.Unsetenv() in tests has been replaced with t.Setenv(), which automatically restores the original environment variable state after the test, preventing side effects between tests.
  • Benchmark Loop Modernization: Go benchmark loops have been updated from for i := 0; i < b.N; i++ to for b.Loop(), aligning with modern Go benchmarking best practices.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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

  1. 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.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • The change from the classic for i := 0; i < b.N; i++ { ... } benchmark loop to for b.Loop() { ... } in BenchmarkHandler_CreateResponse assumes a newer Go version; please confirm the project’s Go toolchain actually supports testing.B.Loop() or keep the old pattern to avoid compilation issues.
  • The new forbidigo rules for context.Background/TODO rely on an issues.exclude-rules text match of use t\. to scope them to tests; consider tightening this (e.g., using a dedicated message or linter ID) so future message wording changes don’t accidentally alter which files are exempted.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The change from the classic `for i := 0; i < b.N; i++ { ... }` benchmark loop to `for b.Loop() { ... }` in `BenchmarkHandler_CreateResponse` assumes a newer Go version; please confirm the project’s Go toolchain actually supports `testing.B.Loop()` or keep the old pattern to avoid compilation issues.
- The new `forbidigo` rules for `context.Background/TODO` rely on an `issues.exclude-rules` text match of `use t\.` to scope them to tests; consider tightening this (e.g., using a dedicated message or linter ID) so future message wording changes don’t accidentally alter which files are exempted.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request is an excellent effort to modernize the Go test suite. The changes systematically replace older testing patterns with newer, safer, and cleaner alternatives provided by the Go testing package. Key improvements include using t.TempDir() for automatic temporary directory cleanup (which also fixes a resource leak), t.Setenv() for managing environment variables, and t.Context() for better cancellation propagation in tests. Additionally, benchmark tests have been updated to use the modern b.Loop() syntax. The inclusion of new forbidigo linter rules in .golangci.yml is a great step to enforce these best practices going forward. The changes are consistent, well-executed across numerous files, and significantly enhance the maintainability and robustness of the tests. The quality of this work is high, and I have no further recommendations for improvement.

@dgageot
Copy link
Member Author

dgageot commented Jan 20, 2026

I'm working on fixing the linter issue. The rules I've added have the nasty side effect of changing the existing forbidigo config

Add lint rules to enforce modern Go test patterns:
- t.TempDir() instead of os.MkdirTemp() in tests
- t.Setenv() instead of os.Setenv() in tests
- t.Context() instead of context.Background()/context.TODO() in tests

The rules use forbidigo with proper pattern syntax (matching identifier
names without parentheses) and are scoped to test files only via an
exclusion rule with 'path-except: _test\.go$'.

Assisted-By: cagent
Copy link
Contributor

@ilopezluna ilopezluna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much @dgageot

@ilopezluna ilopezluna merged commit 7aa2a2f into docker:main Jan 20, 2026
10 checks passed
Copy link
Contributor

@doringeman doringeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants