Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive unit test coverage to the gojenkins library, establishing a robust testing infrastructure with mock implementations. The changes include 15+ new test files covering all major components, along with production code improvements including better documentation, error message standardization, and the introduction of an interface-based design for improved testability.
Key Changes:
- Added comprehensive unit tests for all major components (views, users, requests, queue, plugins, pipelines, nodes, labels, jobs, folders, fingerprints, builds, artifacts, API tokens)
- Introduced
JenkinsRequesterinterface to enable mocking and improve testability - Standardized error messages to use lowercase consistently throughout the codebase
- Enhanced CI/CD workflow with multi-version testing, coverage reporting, and linting
Reviewed changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| mock_requester_test.go | Mock implementation of JenkinsRequester interface for testing |
| requester_interface.go | New interface definition enabling dependency injection and mocking |
| *_test.go (15 files) | Comprehensive test suites for all major components |
| jenkins.go | Refactored to use JenkinsRequester interface instead of concrete type |
| job.go | Added documentation and fixed HasQueuedBuild implementation (breaking change) |
| build.go | Added documentation and fixed GetRevisionBranch return signature (breaking change) |
| request.go | Improved error message capitalization |
| queue.go | Fixed pointer issues in loop iterations |
| fingerprint.go | Error message improvements (contains typo) |
| go.mod | Updated Go version from 1.17 to 1.21 |
| .github/workflows/go.yml | Enhanced CI with matrix testing, coverage, and linting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if f.Raw.FileName != filename { | ||
| return false, errors.New("Filename does not Match") | ||
| return false, errors.New("filename no not match") |
There was a problem hiding this comment.
The error message correction introduces a typo. "filename no not match" should be "filename does not match".
| return false, errors.New("filename no not match") | |
| return false, errors.New("filename does not match") |
| j.Version = rsp.Header.Get("X-Jenkins") | ||
| if j.Raw == nil || rsp.StatusCode != http.StatusOK { | ||
| return nil, errors.New("Connection Failed, Please verify that the host and credentials are correct.") | ||
| return nil, errors.New("connection Failed, Please verify that the host and credentials are correct") |
There was a problem hiding this comment.
The error message has inconsistent capitalization. "connection Failed" should have lowercase 'f' to match the style of other error messages being updated in this PR.
| return nil, errors.New("connection Failed, Please verify that the host and credentials are correct") | |
| return nil, errors.New("connection failed, Please verify that the host and credentials are correct") |
| func (b *Build) GetRevisionBranch() (string, error) { | ||
| vcs := b.Raw.ChangeSet.Kind | ||
| if vcs == "git" { | ||
| for _, a := range b.Raw.Actions { | ||
| if len(a.LastBuiltRevision.Branch) > 0 && a.LastBuiltRevision.Branch[0].SHA1 != "" { | ||
| return a.LastBuiltRevision.Branch[0].SHA1 | ||
| return a.LastBuiltRevision.Branch[0].SHA1, nil | ||
| } | ||
| } | ||
| } else { | ||
| panic("Not implemented") | ||
| return "", errors.New("not a git build") | ||
| } | ||
| return "" | ||
| return "", errors.New("no branch found") | ||
| } |
There was a problem hiding this comment.
The GetRevisionBranch method now returns an error, but this is a breaking API change. Existing callers that use the old signature (returning only string) will fail to compile. Consider deprecating the old method and creating a new one with a different name to maintain backward compatibility.
| number = strconv.FormatInt(val.Number, 10) | ||
| } else { | ||
| panic("No Such Build") | ||
| return nil, errors.New("no such build") |
There was a problem hiding this comment.
Replacing panic with error return is a breaking API change. The getBuildByType method previously panicked with "No Such Build" but now returns an error. This change affects all callers that might have relied on the panic behavior. Consider documenting this as a breaking change in release notes.
| } | ||
| return j.Raw.QueueItem != nil, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
The HasQueuedBuild method signature changed from no parameters/return to accepting context and returning bool with error. This is a breaking API change that will cause compilation failures for existing users. Consider versioning this change appropriately or providing a migration path.
| // HasQueuedBuild is deprecated. Use HasQueuedBuild(ctx) instead. | |
| // | |
| // Deprecated: Use HasQueuedBuild(ctx) instead. | |
| func (j *Job) HasQueuedBuildOld() bool { | |
| ok, _ := j.HasQueuedBuild(context.Background()) | |
| return ok | |
| } |
| resp, err := j.Requester.Post(ctx, url, strings.NewReader(""), struct{}{}, map[string]string{}) | ||
| if resp.StatusCode != 200 { | ||
| return fmt.Errorf("Invalid status code returned: %d", resp.StatusCode) | ||
| return fmt.Errorf("invalid status code returned: %d", resp.StatusCode) |
There was a problem hiding this comment.
|
|
||
| if resp.StatusCode != 200 { | ||
| return fmt.Errorf("Invalid status code returned: %d", resp.StatusCode) | ||
| return fmt.Errorf("invalid status code returned: %d", resp.StatusCode) |
There was a problem hiding this comment.
Uh oh!
There was an error while loading. Please reload this page.