Fix os.Exit in goroutines, Makefile macOS compat, and JSON round-trip tests#107
Open
briansumma wants to merge 6 commits into
Open
Fix os.Exit in goroutines, Makefile macOS compat, and JSON round-trip tests#107briansumma wants to merge 6 commits into
briansumma wants to merge 6 commits into
Conversation
- cmd/profile/authorize.go: Replace os.Exit(1) in goroutine with sending error to result channel, allowing proper cleanup - cmd/pullrequest/create.go: Replace fmt.Fprintf+os.Exit(1) with return err for proper error propagation through Cobra - main.go: Add comment clarifying .env load error is expected Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Makefile: - Inline test recipe to avoid $(COVERAGE_OUT) prerequisite requiring GNU Make 4 features unavailable on macOS default make - Add coverage-report target and restore GOCOV/GOCOVXML variables - Add coverage-report to .PHONY declaration - Use go install with @latest instead of deprecated go get for tools Tests: - cmd/commit: Fix MarshalJSON to omit zero-value fields, fixing round-trip assertion failures in TestCanUnmarshal - cmd/user: Fix MarshalJSON to use time pointer for omitempty - testdata/: Update branch, commit, and pullrequest fixtures to match actual Bitbucket API response structure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Bring develop-pr fully in sync with develop (excluding planning files: CLAUDE.md, .codegraph/, docs/bitbucket-swagger.v3.json). Changes applied from develop: - go.mod/go.sum: update go-flags v0.5.0, go-logger v1.9.5, go-git v5.19.1, and other upstream dep bumps; add gocov/gocov-xml as direct requires - tools/tools.go: pin gocov/gocov-xml via build-tag tools file - cmd/repository/repository.go: add omitempty to HasIssues/HasWiki/IsPrivate - Makefile: exclude tools/ from GOFILES, fix COVERAGE_OUT path, fix coverage-report to use file prereq, add coverage-tools target - cmd/profile, cmd/pullrequest, cmd/pipeline, cmd/issue, cmd/root, etc.: update callers to go-flags v0.5.0 NewEnumFlagWithFunc API (cmd arg added) - version.go, packaging/: version and packaging metadata from master rebase - cmd/common/config.go: new file from upstream - cmd/pullrequest/comment/create_test.go: new test from upstream Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use Author.IsEmpty() instead of an ad-hoc Type/UserID check so authors with only a Raw field (non-Bitbucket-registered users) are not silently dropped from commit JSON output. Add the same [ -x ] existence guard to coverage-report that the test target already uses, so a failed tool installation fails with a clear message rather than a cryptic silent error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t feedback Add TestCanMarshalRawOnlyAuthor to verify that commits from non-Bitbucket- registered users (Author.Raw set, Type and User empty) include the author in marshaled JSON output — the exact regression fixed in the prior commit. Add an else branch to the coverage-report Makefile target so missing coverage tools produce a clear message instead of a silent no-op exit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix
os.Exitcalls in goroutines and error paths —cmd/profile/authorize.gocalledos.Exit(1)inside a goroutine, preventing cleanup;cmd/pullrequest/create.gousedos.Exit(1)instead of returning errors through Cobra. Both now propagate errors properly.Fix Makefile
testtarget for macOS compatibility — Thetesttarget used$(COVERAGE_OUT)as a prerequisite, which relied on GNU Make 4 features unavailable in macOS defaultmake. Inlined the test recipe somake testworks out of the box. Also migrated tool installation from deprecatedgo gettogo installwith@latest.Fix JSON round-trip test failures —
MarshalJSONonCommitandUserstructs emitted zero-value fields (author: {},date: "0001-01-01...") that were absent in the original API fixtures, causingTestCanUnmarshalassertions to fail. Addedomitempty-style nil guards so marshal output matches unmarshal input.Test plan
go build ./...passesgo vet ./...passesTestPullRequestSuite/TestCanUnmarshalpassesTestPullRequestSuite/TestCanUnmarshalWithNilDestinationRepositorypassesmake teston GNU Make 4+)make coverage-reportproduces XML output