Why My First Go PR Failed and What I Learned About Testing and Commit Messages

Hello everyone, I’m Xiao Lou.

Previously I discovered a Go benchmark timeout bug (see my earlier article) and submitted a PR to fix it, hoping to become a Go contributor, but the submission failed. Below is the story of that failure.

First Submission

Go’s source is hosted on Gerrit, not GitHub; a bot mirrors GitHub issues and PRs to Gerrit, and each change must be linked to an issue.

I opened an issue describing the problem, but it was closed as a duplicate of a discussion about a different timeout issue. After some debate, a reviewer pointed out flaws in my code.

Here is the problematic code snippet:

func (b *B) launch() {
    // ...
    // n (int64) may overflow
    n = goalns * prevIters / prevns
    // ...
}

The reviewer said my overflow check was unsafe. He also reminded me to add tests.

Overflow Consideration Incomplete

The reviewer showed a demonstration where a positive overflow becomes negative, then overflows again to positive, indicating that simple checks are insufficient.

Need Tests

He emphasized that the code must be accompanied by tests, even though I thought writing unit tests for benchmarks was hard.

Second Submission

In the second attempt I changed the overflow detection to use reverse arithmetic and added a unit test.

The test sets a 150 s benchmark timeout and aborts after six probe attempts, indicating a problem.

After a long wait, I finally received feedback on the second PR.

Commit Message Not Standard

The reviewer noted that my commit message didn’t follow Go’s guidelines: a short summary line, a blank line, then a detailed description, and a reference to the related issue using Fixes #12345 or similar.

math: improve Sin, Cos and Tan precision for very large arguments The existing implementation has poor numerical properties for large arguments, so use the McGillicutty algorithm to improve accuracy above 1e10. Fixes #159

I need to rewrite my commit messages accordingly.

Consider Integration Tests

The reviewer suggested using integration tests instead of directly calling unexported functions or variables. For example, use flag.Lookup to set flags, or place scripts under cmd/go/testdata/script and read the README for usage.

Missing Comments

Another criticism was the lack of comments, making the code hard to understand. For instance, the benchmark simulates a 150 s run but actually aborts after six attempts, a detail explained in my earlier bug article.

The benchmark can run up to 1e9 iterations, and the probe sequence grows as 100, 10 000, 1 000 000, …, often stopping after a few steps.

Reconsider Overflow Logic

Instead of checking whether n overflows, it may be safer to verify whether goalns exceeds int64_max * prevIters. Using float64 for the whole calculation was also explored, though the results are omitted.

Final Thoughts

Although this PR failed, I learned a lot about Go’s review process, testing strategies, and commit message standards. I’ll keep improving and hope the next submission gets merged.