-
Notifications
You must be signed in to change notification settings - Fork 4
Fix testing #29
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.
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
Fix testing #29
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29 +/- ##
==========================================
+ Coverage 71.88% 72.78% +0.89%
==========================================
Files 23 23
Lines 1615 1683 +68
==========================================
+ Hits 1161 1225 +64
- Misses 371 376 +5
+ Partials 83 82 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EPIC PR, @alvarolivie!! 💪 Of course, I can't evaluate the correctness of the logic (I'd just suggest thorough manual testing), but the code looks pretty good. I didn't spot any obvious issue.
I'm very glad we're now using the specific validations for each CII variant the library supports. And the conditional logic because of the variants, which was one of my concerns, is minimal so far, which supports the idea of keeping everything together in the same package.
There was a problem hiding this 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 adjusts the testing and validation workflows and adapts invoice conversion to support multiple contexts (e.g. Chorus Pro, Factur-X, XRechnung, and PEPPOL). Key changes include modifying how payment term descriptions are concatenated, adding context-based logic in invoice conversion, and updating allowance/charge calculations with adjusted function signatures and conditions.
Reviewed Changes
Copilot reviewed 165 out of 165 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
settlement_parse.go | Changed description assignment to accumulate text |
settlement.go | Updated payment term handling and tax treatments |
party.go | Enhanced party creation for Chorus Pro support |
invoice.go | Added context flag and guideline extraction |
examples_test.go | Updated tests to use new context-specific JSON file paths |
go.mod | Updated dependencies |
cmd/gobl.cii/convert.go | Added flag for selecting conversion context |
allowance_charge.go | Revised allowance/charge creation logic and parameters |
Comments suppressed due to low confidence (2)
allowance_charge.go:118
- This condition assigns a percentage only when Base is non-nil; please confirm that this assumption is intended and document the requirement that a Base must always be provided if a Percent is available.
if c.Percent != nil && c.Base != nil {
agreement.go:41
- [nitpick] The change from 'N/A' to 'NA' may be ambiguous; consider adding a comment or documentation to clarify that 'NA' signifies a default placeholder value.
defaultBuyerReference = "NA"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this 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 enhances the testing framework to run schema and schematron validations across multiple CII contexts (EN16931, FacturX, XRechnung, Peppol, ChorusPro) and fixes related data-handling issues uncovered during validation.
- Added Chorus Pro context support and dynamic guideline extraction
- Reworked settlement terms parsing to include both amount and percent fields
- Overhauled
examples_test.go
to drive per-format tests with--validate
flag and externalxslt3
integration
Reviewed Changes
Copilot reviewed 165 out of 165 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
settlement_parse.go | Changed how term descriptions are concatenated and partial payments are parsed |
settlement.go | Added Amount /Percent fields to Terms and updated summary/due‐date logic |
party.go | Introduced LegalOrganization for ChorusPro, moved email handling to newURIUniversalCommunication |
examples_test.go | Refactored tests for multi‐format validation, added schema/schematron checks |
invoice.go | Added extractGuidelineID for ChorusPro and imported slices for addon detection |
cmd/gobl.cii/convert.go | Added --context flag and mapped formats to correct Context values |
allowance_charge.go | Removed redundant base sum parameter, adjusted percent/base pairing in line charges |
Comments suppressed due to low confidence (3)
examples_test.go:286
- [nitpick] Function
isrootFolder
uses inconsistent capitalization. Consider renaming toisRootFolder
to follow Go naming conventions and improve readability.
for !isrootFolder(cwd) {
examples_test.go:259
- [nitpick] The local variable
schemaPath
shadows the package-levelschemaPath
constant, which can be confusing. Consider renaming one to avoid shadowing and improve clarity.
schemaPath := filepath.Join(rootFolder(), schemaPath, defaultFormat, schemaFile)
settlement.go:118
- Guarding creation of
PaymentTerms
on non-empty descriptions changes prior behavior, which always produced a default term entry. If an empty term is still required, consider removing this guard or explicitly handling the empty case.
if description != "" {
This branch proposes a new way to handle testing that includes schema and schematron validation per type. Testing showed some errors that needed to be fixed. It also adds a new context for Chorus Pro (depends on invopop/gobl#544).
Testing Framework
Originally, this repo only handled schema validation for base CII and did not take into consideration other schemas like FacturX or XRechnung. The repo also ignored schematrons completely. Schemas validate the syntactic aspects while schematrons look at semantics.
The new testing structure in
tools/
provides comprehensive validation for different CII implementations:See the tools README for details on available schemas, schematrons, and how to use the validation tools.
Main Changes
Running the new validation tests revealed several issues that have been fixed as well as some changes to simplify the implementation.
--validate
flag to test against schema and schematron.omitempty
tags to ensure no blank fields are created. Email handled here now.-settlement_parse.go: We now parse the percent which may be present instead of only the amount.
Most fixes relate to schematron validation rules that enforce semantic correctness beyond basic XML schema validation.