chore: harden build config and expand audit test coverage#16
Conversation
inflop
commented
Apr 28, 2026
inflop
commented
- Add .editorconfig with C# naming conventions and formatting rules
- Add Directory.Build.props with TreatWarningsAsErrors in Release
- Remove duplicated MSBuild properties from .csproj files
- Add structural invariant tests (TotalGross = TotalNet + TotalVat, etc.)
- Add rounding invariant tests (AwayFromZero, not Banker's)
- Expand discount and foreign currency test coverage
- Add .editorconfig with C# naming conventions and formatting rules - Add Directory.Build.props with TreatWarningsAsErrors in Release - Remove duplicated MSBuild properties from .csproj files - Add structural invariant tests (TotalGross = TotalNet + TotalVat, etc.) - Add rounding invariant tests (AwayFromZero, not Banker's) - Expand discount and foreign currency test coverage
There was a problem hiding this comment.
Pull request overview
This PR strengthens repo-wide .NET build configuration and expands the test suite with invariants that defend VAT rounding and structural properties across calculation methods (including FCY and discount behaviors).
Changes:
- Added repo-wide build defaults via
.editorconfigandDirectory.Build.props, and removed duplicated per-project MSBuild properties. - Added new invariant-focused test suites for structural totals and midpoint rounding (AwayFromZero).
- Expanded coverage for FCY + discount propagation and additional PerUnit discount scenarios.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Inflop.VatSharp.Tests/RoundingInvariantTests.cs | New tests to lock in AwayFromZero midpoint rounding behavior across methods. |
| tests/Inflop.VatSharp.Tests/InvariantTests.cs | New structural and per-method invariant tests (totals, summaries, idempotency, edge cases). |
| tests/Inflop.VatSharp.Tests/Inflop.VatSharp.Tests.csproj | Removes duplicated nullable/implicit usings (now centralized) and adds a local WAE override. |
| tests/Inflop.VatSharp.Tests/ForeignCurrencyCalculatorTests.cs | Adds FCY tests for PerUnit discount propagation and ToBaseDocumentAmounts behavior. |
| tests/Inflop.VatSharp.Tests/DiscountTests.cs | Adds combinatorial PerUnit discount coverage and simplifies some item construction formatting. |
| tests/Inflop.VatSharp.Tests/DirectEngineTests.cs | Adds an assertion clarifying defining behavior difference between Method I and Method III. |
| src/Inflop.VatSharp/Inflop.VatSharp.csproj | Removes duplicated nullable/implicit usings (now in Directory.Build.props). |
| Directory.Build.props | Introduces central compiler/analyzer settings and Release-only warnings-as-errors. |
| .editorconfig | Adds formatting + naming rules for consistent C# style and analyzer behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| // Tests that discountBehavior is properly propagated from | ||
| // VatCalculationEngine.Create(...) through to the FCY pipeline. | ||
| var engine = VatCalculationEngine.Create(discountBehavior: Strategies.Discount.PerUnitAbsoluteDiscountBehavior.Instance); |
There was a problem hiding this comment.
Strategies.Discount.PerUnitAbsoluteDiscountBehavior.Instance is not in scope here (there’s no using Inflop.VatSharp.Strategies.Discount; and Strategies isn’t a top-level namespace), so this won’t compile. Use the fully-qualified Inflop.VatSharp.Strategies.Discount.PerUnitAbsoluteDiscountBehavior.Instance or add the appropriate using and reference PerUnitAbsoluteDiscountBehavior.Instance directly.
| var engine = VatCalculationEngine.Create(discountBehavior: Strategies.Discount.PerUnitAbsoluteDiscountBehavior.Instance); | |
| var engine = VatCalculationEngine.Create(discountBehavior: Inflop.VatSharp.Strategies.Discount.PerUnitAbsoluteDiscountBehavior.Instance); |
| tab_width = 4 | ||
|
|
||
| # New line preferences | ||
| end_of_line = crlf |
There was a problem hiding this comment.
The file sets end_of_line = lf for [*] but then overrides C# files to end_of_line = crlf. This creates mixed line endings across the repo and can cause unnecessary diffs/format churn. Consider using a single line-ending policy (commonly lf everywhere) and remove the conflicting override.
| end_of_line = crlf | |
| end_of_line = lf |
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <Nullable>enable</Nullable> | ||
| <IsPackable>false</IsPackable> | ||
| <TreatWarningsAsErrors>false</TreatWarningsAsErrors> |
There was a problem hiding this comment.
This project explicitly sets <TreatWarningsAsErrors>false</TreatWarningsAsErrors>, which overrides the repo-wide Release setting in Directory.Build.props and weakens the intended "warnings as errors in Release" hardening. If the goal is to keep tests exempt, consider making this conditional (e.g., only in Debug) or suppressing specific warnings instead of disabling the policy entirely.
| <TreatWarningsAsErrors>false</TreatWarningsAsErrors> | |
| <TreatWarningsAsErrors Condition="'$(Configuration)' == 'Debug'">false</TreatWarningsAsErrors> |
