Skip to content

Refactor test helpers and upgrade gobl to v0.401#66

Open
samlown wants to merge 3 commits into
mainfrom
refactor-test-helpers-and-gobl-upgrade
Open

Refactor test helpers and upgrade gobl to v0.401#66
samlown wants to merge 3 commits into
mainfrom
refactor-test-helpers-and-gobl-upgrade

Conversation

@samlown
Copy link
Copy Markdown
Contributor

@samlown samlown commented Apr 22, 2026

Summary

  • Test helper refactor: parseXMLInvoice (renamed from testParseInvoice), testInvoiceFrom, and loadTestEnvelope now all take *testing.T, call t.Helper(), and fail the test via require.NoError internally instead of returning errors that every caller re-asserted. This collapses ~60 call sites of the form x, err := helper(...); require.NoError(t, err) down to a single-line x := helper(t, ...).
  • Dependency upgrade: bumps github.com/invopop/gobl from v0.309.0 to v0.401.0, with ensureAddons in ubl.Convert adjusted to operate on the envelope so Calculate/Validate run at the envelope level.

Test plan

  • go vet ./... passes
  • go test ./... passes on the target gobl version
  • Spot-check that helper failures still point to the calling test (via t.Helper())

🤖 Generated with Claude Code

samlown and others added 2 commits April 22, 2026 16:31
Bump gobl dependency from v0.309.0 to v0.401.0 (plus the resulting
transitive updates), and adjust ensureAddons in ubl.Convert to operate
on the envelope instead of the extracted invoice so that calculate/
validate are invoked at the envelope level.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rename testParseInvoice to parseXMLInvoice, and adopt a consistent
style across the parse-test helpers (parseXMLInvoice, testInvoiceFrom,
loadTestEnvelope): each now takes *testing.T, calls t.Helper(), and
uses require.NoError internally instead of returning errors that every
caller immediately re-asserted.

This collapses the ubiquitous two-line `x, err := helper(...); require.NoError(t, err)`
pattern down to a single `x := helper(t, ...)`, removes the now-unused
require import from four test files, and keeps test failure reporting
anchored at the caller via t.Helper.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 22, 2026

Codecov Report

❌ Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.47%. Comparing base (c2149f3) to head (5fefcce).

Files with missing lines Patch % Lines
ubl.go 81.25% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #66      +/-   ##
==========================================
+ Coverage   76.03%   76.47%   +0.44%     
==========================================
  Files          26       26              
  Lines        1928     2589     +661     
==========================================
+ Hits         1466     1980     +514     
- Misses        336      489     +153     
+ Partials      126      120       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alvarolivie alvarolivie requested a review from Copilot April 22, 2026 17:25
Copy link
Copy Markdown
Contributor

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 refactors test helper functions to take *testing.T, use t.Helper(), and fail tests internally (reducing repetitive require.NoError call sites), and upgrades github.com/invopop/gobl to the v0.401.x line with an update to addon handling during conversion.

Changes:

  • Refactor test helpers (testInvoiceFrom, parseXMLInvoice, loadTestEnvelope) to accept *testing.T and assert internally.
  • Update ubl.Convert addon enforcement to run Calculate/Validate at the envelope level.
  • Bump dependencies (gobl, testify, and indirects) via go.mod/go.sum.

Reviewed changes

Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
ubl.go Moves addon enforcement to operate on the envelope before UBL conversion.
examples_test.go Refactors core test helpers to take *testing.T and fail internally.
totals_test.go Updates callers to new helper signatures.
payment_test.go Updates callers to new helper signatures and removes now-unused require import.
payment_parse_test.go Updates XML parsing tests to use parseXMLInvoice(t, ...).
party_test.go Updates callers to new helper signature and removes now-unused require import.
party_parse_test.go Updates XML parsing tests to use parseXMLInvoice(t, ...).
ordering_test.go Updates callers to new helper signature and removes now-unused require import.
ordering_parse_test.go Updates XML parsing tests to use parseXMLInvoice(t, ...).
lines_test.go Updates callers to new helper signature and removes now-unused require import.
lines_parse_test.go Updates XML parsing tests to use parseXMLInvoice(t, ...).
invoice_test.go Updates callers to new helper signature for loading envelopes.
invoice_parse_test.go Updates XML parsing tests to use parseXMLInvoice(t, ...).
delivery_test.go Updates callers to new helper signature for loading envelopes.
delivery_parse_test.go Updates XML parsing tests to use parseXMLInvoice(t, ...).
context_test.go Updates callers to new helper signature for loading envelopes.
charges_test.go Updates callers to new helper signature and removes now-unused require import.
charges_parse_test.go Updates XML parsing tests to use parseXMLInvoice(t, ...).
attachments_test.go Updates callers to new helper signature for invoice loading.
attachments_parse_test.go Updates XML parsing tests to use parseXMLInvoice(t, ...).
go.mod Upgrades gobl and testify, plus indirect dependency updates.
go.sum Updates module checksums for upgraded direct/indirect dependencies.

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

Comment thread ubl.go
Comment on lines +126 to +128
inv, ok := env.Extract().(*bill.Invoice)
if !ok {
return fmt.Errorf("expected bill.Invoice, got %T", env.Extract())
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

ensureAddons calls env.Extract() twice (once for the type assertion and again in the error). It’d be cleaner and avoids any repeated work to extract once into a doc variable and reuse it both for the assertion and the %T in the error message.

Suggested change
inv, ok := env.Extract().(*bill.Invoice)
if !ok {
return fmt.Errorf("expected bill.Invoice, got %T", env.Extract())
doc := env.Extract()
inv, ok := doc.(*bill.Invoice)
if !ok {
return fmt.Errorf("expected bill.Invoice, got %T", doc)

Copilot uses AI. Check for mistakes.
Comment thread examples_test.go
path := filepath.Join(getConversionTypePath(jsonPattern), name)
src, _ := os.Open(path)
src, err := os.Open(path)
require.NoError(t, err)
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

loadTestEnvelope opens the file but never closes it. Since this helper is used widely across tests, this can leak file descriptors and cause flaky failures on some platforms/CI. Add a defer src.Close() (ignoring/handling the close error consistently with the rest of the file) after os.Open.

Suggested change
require.NoError(t, err)
require.NoError(t, err)
defer func() {
require.NoError(t, src.Close())
}()

Copilot uses AI. Check for mistakes.
Comment thread ubl.go
Comment on lines +103 to +109
// Check and add missing addons
if err := ensureAddons(env, o.context.Addons); err != nil {
return nil, err
}

switch doc := env.Extract().(type) {
case *bill.Invoice:
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

Convert calls ensureAddons before checking the extracted document type. Because all contexts currently have non-empty Addons, passing an envelope that isn't a *bill.Invoice will now return the ensureAddons type error instead of ErrUnsupportedDocumentType, which is a behavior change for unsupported docs. Consider moving the ensureAddons call inside the case *bill.Invoice branch (or make ensureAddons a no-op for non-invoice documents) so unsupported types consistently return ErrUnsupportedDocumentType.

Suggested change
// Check and add missing addons
if err := ensureAddons(env, o.context.Addons); err != nil {
return nil, err
}
switch doc := env.Extract().(type) {
case *bill.Invoice:
switch doc := env.Extract().(type) {
case *bill.Invoice:
// Check and add missing addons
if err := ensureAddons(env, o.context.Addons); err != nil {
return nil, err
}

Copilot uses AI. Check for mistakes.
@samlown samlown requested a review from alvarolivie April 23, 2026 21:35
Copy link
Copy Markdown
Collaborator

@alvarolivie alvarolivie left a comment

Choose a reason for hiding this comment

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

LGTM, just one thing I'd double check

Comment thread go.mod

require (
github.com/invopop/gobl v0.309.0
github.com/invopop/gobl v0.401.1-0.20260422142243-5cf58e92818c
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this gobl version expected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants