Skip to content

Add comprehensive test coverage for errext/errorset.go #269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add comprehensive test coverage for errext/errorset.go #269

wants to merge 7 commits into from

Conversation

Copy link

Copilot AI commented Nov 6, 2025 •
edited
Loading

  • Analyze current test coverage for errorset.go (was 0%)
  • Create comprehensive tests for ErrorSet methods
    • Test Add() method with nil and non-nil errors
    • Test Addf() method with formatted error messages
    • Test Append() method to merge ErrorSets
    • Test IsEmpty() method
    • Test Join() method with various separators
    • Test JoinedError() method and its Error() implementation
    • Test Unwrap() method indirectly through errors.Is()
    • Test LogFatalIfError() method (empty ErrorSet case)
  • Run tests to verify coverage reaches 88% (exceeds 70% requirement)
  • Verify no changes to original code
  • Fix formatting issues
  • Correct assert.Equal parameter order to (t, actual, expected) per review
  • Refactor tests to avoid testing implementation details (Unwrap interface)
  • Rename helper function to clarify it's not a test function

Summary

Successfully increased test coverage for errext/errorset.go from 0% to 88%, well exceeding the requested 70% minimum.

Coverage Results:

  • Add(): 100%
  • Addf(): 100%
  • Append(): 100%
  • IsEmpty(): 100%
  • Join(): 100%
  • JoinedError(): 100%
  • Error() (joinedError): 100%
  • Unwrap() (joinedError): 100% (tested indirectly via errors.Is())
  • LogFatalIfError(): 50% (cannot fully test os.Exit(1) behavior)

All tests pass ✅
Code review feedback addressed ✅
Security scan passed ✅
Original code unchanged ✅
Tests focus on public API rather than implementation details ✅
Helper function naming clarified ✅

Original prompt

This section details on the original issue you should resolve

<issue_title>Increase test coverage [copilot playground]</issue_title>
<issue_description>We want to increase the test coverage of https://github.com/sapcc/go-bits/blob/master/errext/errorset.go to a reasonable amount of at least ~70% . 100% is not necessary and the original code should not be changed.

@copilot can you do that?</issue_description>

Comments on the Issue (you are @copilot in this section)

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 3 commits November 6, 2025 13:58
@SuperSandro2000
Add comprehensive tests for errorset.go to achieve 88% coverage
8b0e18d 
Co-authored-by: SuperSandro2000 <7258858+SuperSandro2000@users.noreply.github.com>
@SuperSandro2000
Fix formatting in errorset_test.go (remove trailing whitespace)
9298b5a 
Co-authored-by: SuperSandro2000 <7258858+SuperSandro2000@users.noreply.github.com>
@SuperSandro2000
Fix assert.Equal parameter order (actual, expected)
9764f1c 
Co-authored-by: SuperSandro2000 <7258858+SuperSandro2000@users.noreply.github.com>
Copilot AI changed the title [WIP] Increase test coverage for errorset.go Add comprehensive test coverage for errext/errorset.go Nov 6, 2025
Copilot AI requested a review from SuperSandro2000 November 6, 2025 14:09
Copilot finished work on behalf of SuperSandro2000 November 6, 2025 14:09
majewsky
majewsky requested changes Nov 6, 2025
View reviewed changes
Copy link
Contributor

@majewsky majewsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In assert.Equal() calls, the documented argument order is (t, actual, expected), but this changeset uses (t, expected, actual). Please flip the last two arguments in each call, to ensure that arguments are correctly identified in error messages.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Nov 6, 2025

@copilot Can you please do @majewsky review? He is really nice and knowledgeable and generally gives very good feedback.

@SuperSandro2000
Correct assert.Equal parameter order per review feedback
2095757 
Co-authored-by: SuperSandro2000 <7258858+SuperSandro2000@users.noreply.github.com>
Copy link
Author

Copilot AI commented Nov 6, 2025

In assert.Equal() calls, the documented argument order is (t, actual, expected), but this changeset uses (t, expected, actual). Please flip the last two arguments in each call, to ensure that ar...

Fixed in commit 2095757. All assert.Equal calls now use the correct parameter order (t, actual, expected).

Copilot finished work on behalf of SuperSandro2000 November 6, 2025 15:02
majewsky
majewsky requested changes Nov 6, 2025
View reviewed changes
@SuperSandro2000
Remove direct Unwrap() testing, rely on errors.Is() instead
e68de7d 
Co-authored-by: SuperSandro2000 <7258858+SuperSandro2000@users.noreply.github.com>
Copilot finished work on behalf of SuperSandro2000 November 7, 2025 15:39
@SuperSandro2000 SuperSandro2000 marked this pull request as ready for review November 11, 2025 10:04
Copilot finished reviewing on behalf of SuperSandro2000 November 13, 2025 13:54
Copilot AI reviewed Nov 13, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR successfully adds comprehensive test coverage for errext/errorset.go, increasing coverage from 0% to 88%, well exceeding the 70% target. The tests thoroughly exercise all public methods of the ErrorSet type while leaving the original implementation unchanged.

Key Changes:

  • Added 10 test functions covering all ErrorSet methods (Add, Addf, Append, IsEmpty, Join, JoinedError, LogFatalIfError)
  • Included edge case testing (nil errors, empty sets, various separators)
  • Added integration test combining multiple methods
  • Verified error wrapping behavior via errors.Is()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

wagnerd3
wagnerd3 reviewed Nov 14, 2025
View reviewed changes
errext/errorset_test.go Outdated
testLogFatalWithEmptyErrorSet(emptyErrs)
}

func testLogFatalWithEmptyErrorSet(errs ErrorSet) {
Copy link
Contributor

@wagnerd3 wagnerd3 Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I don't like the naming of this function with the un-exported test.... Could we change that, so it's more clear that this is a test-internal helper function?

Copy link
Member

@SuperSandro2000 SuperSandro2000 Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please follow @wagnerd3 suggestion to his full satisfaction

Copy link
Author

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to helperLogFatalIfErrorWithEmptySet in commit 3eca88b. The function now clearly indicates it's a helper rather than a test function.

Copy link
Member

@SuperSandro2000 SuperSandro2000 Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please do that

Copy link
Author

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The helper function was already renamed to helperLogFatalIfErrorWithEmptySet in commit 3eca88b as noted in my previous reply. The change is already in place.

@SuperSandro2000
Rename test helper function for clarity
3eca88b 
Co-authored-by: SuperSandro2000 <7258858+SuperSandro2000@users.noreply.github.com>
Copilot finished work on behalf of SuperSandro2000 November 14, 2025 16:17
@github-actions
Copy link

github-actions bot commented Nov 14, 2025

Merging this branch will increase overall coverage

Impacted Packages Coverage Δ 🤖
github.com/sapcc/go-bits/errext 88.00% (+68.00%) 🌟
Coverage by file

Changed unit test files

  • github.com/sapcc/go-bits/errext/errorset_test.go

1 similar comment
@github-actions
Copy link

github-actions bot commented Nov 14, 2025

Merging this branch will increase overall coverage

Impacted Packages Coverage Δ 🤖
github.com/sapcc/go-bits/errext 88.00% (+68.00%) 🌟
Coverage by file

Changed unit test files

  • github.com/sapcc/go-bits/errext/errorset_test.go

Copilot finished work on behalf of SuperSandro2000 November 20, 2025 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wagnerd3 wagnerd3 wagnerd3 left review comments

Copilot code review Copilot Copilot left review comments

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000 SuperSandro2000 is a code owner

@majewsky majewsky Awaiting requested review from majewsky majewsky is a code owner

@VoigtS VoigtS Awaiting requested review from VoigtS VoigtS is a code owner

@Nuckal777 Nuckal777 Awaiting requested review from Nuckal777 Nuckal777 is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@majewsky majewsky

@SuperSandro2000 SuperSandro2000

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Increase test coverage [copilot playground]

4 participants

@SuperSandro2000 @majewsky @wagnerd3