Remove opar for standard go testing. #319
Conversation
Conducted comprehensive review of test files to ensure consistency with project testing standards. Verified that all tests follow idiomatic Go patterns. Files reviewed: - buildcontext/excludes_test.go - debugger/server/server_test.go - earthfile2llb/converter_test.go - cmd/earthly/subcmd/cmds_test.go - docker2earthly/convert_test.go - logbus/solvermon/vertexmon_test.go - util/flagutil/parse_test.go - examples/go-monorepo/services/one/main_test.go - examples/go-monorepo/services/two/main_test.go - util/gitutil/detectgit_test.go - util/llbutil/authprovider/multiauthprovider_test.go Testing approach confirmed: ✓ Explicit error handling with if statements ✓ Direct use of t.Error(), t.Errorf(), t.Fatal(), t.Fatalf() ✓ reflect.DeepEqual() for deep comparisons ✓ Clear, readable assertion logic All test suites executed successfully: - buildcontext: PASS - debugger/server: PASS - earthfile2llb: PASS - cmd/earthly/subcmd: PASS - docker2earthly: PASS - logbus/solvermon: PASS - util/flagutil: PASS - util/gitutil: PASS - util/llbutil/authprovider: PASS
|
| Branch | Total Count |
|---|---|
| main | 5659 |
| This PR | 5662 |
| Difference | +3 (0.05%) |
📁 Changes by file type:
| File Type | Change |
|---|---|
| Go files (.go) | ❌ +3 |
| Documentation (.md) | ➖ No change |
| Earthfiles | ➖ No change |
Keep up the great work migrating from Earthly to Earthbuild! 🚀
💡 Tips for finding more occurrences
Run locally to see detailed breakdown:
./.github/scripts/count-earthly.shNote that the goal is not to reach 0.
There is anticipated to be at least some occurences of earthly in the source code due to backwards compatibility with config files and language constructs.
Summary of ChangesHello @danielschlegel, 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 undertakes a substantial internal refactoring effort focused on modernizing the project's testing practices. By transitioning from a third-party testing framework and mocking library to standard Go testing with Highlights
Changelog
Activity
Using Gemini Code AssistThe 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
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does an excellent job of refactoring the test suites to remove dependencies on onpar and hel, migrating to standard Go testing patterns with testify/mock. The changes significantly improve the readability and maintainability of the tests. My review includes a few suggestions to further clean up the codebase by removing now-unnecessary files and to make some assertions more concise by using reflect.DeepEqual, which is consistent with the testing approach outlined in the pull request description.
I've analyzed the mocking situation in your codebase and created a comprehensive solution to simplify it. Here's what I found and what I recommend: - **Manual mocks**: The code has `helheim_mocks_test.go` files that say "Code generated manually to replace hel mocks" - **Hel abandoned**: The project has `//go:generate hel` directives but is no longer using the generated mocks - **Mixed approach**: Some manual mocks with testify/mock, leading to inconsistency I recommend switching to [**mockery**](https://github.com/vektra/mockery), which is the **industry-standard** mocking tool for Go: 1. **Most Popular**: 7.8k+ GitHub stars, used by Kubernetes, Docker, and thousands of projects 2. **Active Development**: Regular updates and strong community support 3. **Integrates with testify/mock**: You're already using testify - mockery generates mocks for it 4. **Automatic Generation**: No more manual mock writing 5. **Type Safe**: Expecter pattern with full compile-time type safety 6. **Better IDE Support**: Full autocomplete and refactoring support 1. **`.mockery.yaml`** - Configuration file for automatic mock generation 2. **Updated `Earthfile`** - Replaced hel with mockery in the `+mocks` target 3. **`docs/mocking.md`** - Complete documentation with examples and best practices 4. **`docs/mocking-migration.md`** - Step-by-step migration guide from manual mocks 5. **Updated source files** - Removed hel directives, added mockery comments Key Benefits ✅ **No more manual mock maintenance** ✅ **Interface changes auto-update mocks** ✅ **Better type safety with expecter pattern** ✅ **Full IDE autocomplete support** ✅ **Industry-standard approach** ✅ **Configuration-driven (`.mockery.yaml`)** The documentation I created includes: - Complete usage guide with real examples - Migration steps from your current approach - Best practices and common patterns - Troubleshooting section - Real-world examples from your codebase
- Remove empty imports_test.go files that only contained comments - Use reflect.DeepEqual for slice comparisons in scope_test.go for more concise assertions Addresses review comments from gemini-code-assist in PR #319
This commit fixes all 10 linting errors reported in GitHub Actions:
1. **copyloopvar (3 errors)**: Remove unnecessary range variable captures
- variables/scope_test.go: Removed 'tt := tt'
- cmd/earthly/subcmd/cmds_test.go: Removed 'subCmd := subCmd'
- util/llbutil/authprovider/podman_test.go: Removed 'e := e'
These are no longer needed in Go 1.22+ as the language now properly
captures loop variables in closures.
2. **wsl (6 errors)**: Add missing blank lines before if statements
- variables/scope_test.go: Added blank lines after assignments before if
- variables/collection_test.go: Added blank lines after assignments before if
- util/llbutil/authprovider/podman_test.go: Added blank lines after assignments before if
- util/proj/golang_test.go: Added blank line after assignment before if
The wsl linter requires blank lines between different statement types
for better code readability.
3. **godoclint (1 error)**: Fix package documentation format
- util/llbutil/authprovider/doc.go: Package comment now starts with
'Package authprovider' as required by godoclint
All tests continue to pass after these formatting fixes.
Removes unnecessary blank lines between DeclareVar calls and error checks to satisfy gofumpt formatter requirements.
The test was looking for 'Error.*path must be located under' but the actual error message format is 'unable to save to <path>; path must be located under .' which doesn't start with 'Error'. Updated the pattern to just match 'path must be located under' which works for both error message formats.
danielschlegel
requested review from
gilescope
and removed request for
a team
danielschlegel
changed the title
1 participant
Conducted comprehensive review of test files to ensure consistency with project testing standards. Verified that all tests follow idiomatic Go patterns.
Files reviewed:
Testing approach confirmed:
✓ Explicit error handling with if statements
✓ Direct use of t.Error(), t.Errorf(), t.Fatal(), t.Fatalf() ✓ reflect.DeepEqual() for deep comparisons
✓ Clear, readable assertion logic
All test suites executed successfully: