chore: replace Python test harness with Go implementation#2915
chore: replace Python test harness with Go implementation#2915markphelps wants to merge 2 commits intomainfrom
Conversation
Signed-off-by: Mark Phelps <mphelps@cloudflare.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| if err != nil { | ||
| return "", fmt.Errorf("creating binary file: %w", err) | ||
| } | ||
| defer f.Close() |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR replaces the Python test harness with a Go implementation, converting tools under tools/ to standalone Go modules. The implementation is solid overall with good package structure and proper use of errgroup for concurrent builds.
I found one actionable issue:
Unchecked error from g.Wait() - The error return from g.Wait() at line 270 is discarded. While the goroutines handle their own errors via captured variables, the WaitGroup itself can return context cancellation errors that would be lost.
| }) | ||
|
|
||
| g.Wait() |
There was a problem hiding this comment.
Unchecked error from g.Wait(). Context cancellation errors would be lost here.
| }) | |
| g.Wait() | |
| if err := g.Wait(); err != nil { | |
| result.Passed = false | |
| result.Error = fmt.Sprintf("context cancelled: %v", err) | |
| return result | |
| } |
|
remote: Permission to replicate/cog.git denied to github-actions[bot]. |
|
@markphelps Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR replaces the Python test harness with a Go implementation, converting tools under tools/ to standalone Go modules. The implementation follows Go best practices with good package separation and proper use of errgroup for concurrency.
I found two actionable issues:
-
Unchecked error from
g.Wait()- The error return fromg.Wait()is discarded. While goroutines handle their own errors via captured variables, the WaitGroup itself can return context cancellation errors that would be lost. -
File close without error handling - The file handle opened with
os.OpenFileis closed withdefer f.Close()which ignores errors. For a file opened with write permissions, close errors could indicate data loss. Although the checksum verification would likely catch this, explicit error handling is preferred.
| return nil | ||
| }) | ||
|
|
||
| g.Wait() |
There was a problem hiding this comment.
Unchecked error from g.Wait(). Context cancellation errors would be lost here.
| g.Wait() | |
| if err := g.Wait(); err != nil { | |
| result.Passed = false | |
| result.Error = fmt.Sprintf("context cancelled: %v", err) | |
| return result | |
| } |
| if err != nil { | ||
| return "", fmt.Errorf("creating binary file: %w", err) | ||
| } | ||
| defer f.Close() |
There was a problem hiding this comment.
File handle opened for writing should have explicit close error handling. Consider using named returns and a deferred function to capture close errors:
func downloadCogBinary(tag string) (dest string, err error) {
// ... existing code ...
f, err := os.OpenFile(dest, os.O_CREATE|os.O_WRONLY, 0755)
if err != nil {
return "", fmt.Errorf("creating binary file: %w", err)
}
defer func() {
if closeErr := f.Close(); closeErr != nil && err == nil {
err = fmt.Errorf("closing binary file: %w", closeErr)
}
}()
// ... rest of function ...
}
|
Review submitted successfully to PR #2915. The review includes suggestions for:
|
1 participant
Summary
tools/test-harnessPython implementation with a full Go implementation (cmd/+internal/packages) while preserving manifest/fixture-driven workflowsrun,build,list, andschema-comparecommands, including resolver/reporting/validator logic and new Go unit teststools/to standalone Go modules and update task invocations inmise.tomlwhere neededValidation
go test ./...(intools/test-harness)go run . list(intools/test-harness)go test ./...in each standalone tool module (tools/compatgen,tools/gendocs,tools/test-registry-util,tools/version,tools/weights-gen)