[PM-28610]Resolve the Failing InvoiceExtensionsTests #6622
Conversation
|
Claude finished @cyprain-okeke's task —— View job Code ReviewSummaryThis PR fixes a failing test by wrapping the test method in a culture-specific context to ensure consistent decimal formatting across different system locales. The test was failing because decimal formatting (e.g., FindingsFinding 1: Test expectation change requires verification 💭 The test expectation changed from Finding 2: Culture setting approach could be more robust 🎨 Using a try-finally block with culture restoration is the correct approach. However, consider using Finding 3: PR description gaps
Finding 4: Consider checking for similar issues 💭 Other tests in this file that format currency values might have similar culture-dependency issues. Consider reviewing tests like Good Practices Observed
Action Items
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| try | ||
| { | ||
| Thread.CurrentThread.CurrentCulture = new CultureInfo("en-US"); | ||
| Thread.CurrentThread.CurrentUICulture = new CultureInfo("en-US"); |
There was a problem hiding this comment.
🎨 Consider using CultureInfo.InvariantCulture instead of new CultureInfo("en-US") for unit tests. Invariant culture is specifically designed for culture-independent operations and makes the intent clearer - you want consistent formatting regardless of system locale, not specifically US formatting.
| Thread.CurrentThread.CurrentUICulture = new CultureInfo("en-US"); | |
| Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture; | |
| Thread.CurrentThread.CurrentUICulture = CultureInfo.InvariantCulture; |
| Assert.Equal(5, result.Count); | ||
| Assert.Equal("5 × Manage service provider (at $6.00 / month)", result[0]); | ||
| Assert.Equal("10 × Manage service provider (at $4.00 / month)", result[1]); | ||
| Assert.Equal("1 × Tax (at $8.00 / month)", result[2]); |
There was a problem hiding this comment.
💭 This assertion now expects $8.00, which correctly matches the test data (line 381: Amount = 800 cents = $8.00). The original test expected $0.00.
Can you confirm this change represents correct business logic? Was the original $0.00 expectation:
- A test data error that should have been 0 cents?
- An intentional regression test?
- Or is
$8.00the correct expected behavior?
This is important to verify we're fixing the test correctly, not just making it pass.
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6622 +/- ##
==========================================
- Coverage 53.00% 52.99% -0.01%
==========================================
Files 1909 1909
Lines 84828 84828
Branches 7634 7634
==========================================
- Hits 44961 44955 -6
- Misses 38115 38119 +4
- Partials 1752 1754 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-28610
📔 Objective
Fix this error below :
FormatForProvider_ComplexScenario_HandlesAllLineTypes [1 ms]
Assert.Equal() Failure: Strings differ
Expected: "1 × Tax (at $0.00 / month)"
Actual: "1 × Tax (at $8.00 / month)"
↑ (pos 14)
at Bit.Core.Test.Billing.Extensions.InvoiceExtensionsTests
.FormatForProvider_ComplexScenario_HandlesAllLineTypes()
in /Users/oscar/Code/server/test/Core.Test/Billing/Extensions/InvoiceExtensionsTests.cs:line 389
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes