test: apply AAA pattern and DAMP to SentryByteCountFormatterTests #7221
Conversation
Refactored SentryByteCountFormatterTests to follow Arrange-Act-Assert pattern with explicit comment markers. Inlined helper method to make tests DAMP (Descriptive And Meaningful Phrases) - each test is now self-contained and readable without jumping to helper methods. Added descriptive constants (midRangeMultiplier, maxValueOffset) and variable names (singleUnitResult, midRangeResult, maxValueResult) to eliminate magic numbers and improve clarity.
philipphofmann
requested review from
itaybre,
noahsmartin and
philprime
as code owners
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog. This PR will not appear in the changelog. 🤖 This preview updates automatically when you update the PR. |
philipphofmann
added
the
ready-to-merge
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7221 +/- ##
=============================================
+ Coverage 85.062% 85.076% +0.014%
=============================================
Files 468 468
Lines 28305 28305
Branches 12532 12536 +4
=============================================
+ Hits 24077 24081 +4
+ Misses 4183 3962 -221
- Partials 45 262 +217 see 28 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Removed maxValueOffset constant that incorrectly changed test semantics. The original expression 'baseValue * 1_024 - 1' evaluates as '(baseValue * 1_024) - 1', testing true boundary values. The extracted constant 'maxValueOffset = 1_024 - 1' changed this to 'baseValue * 1_023', producing incorrect test values. Addresses feedback: #7221 (comment)
philipphofmann
force-pushed
the
test/aaa-pattern-damp-byte-count-formatter
branch
2 times, most recently
from
0db46aa to
abb377a
Compare
|
Fixed! Changed from the incorrect constant extraction to using inline expression Also updated constant names to |
Refactored SentryByteCountFormatterTests to follow Arrange-Act-Assert pattern with explicit comment markers. Inlined helper method to make tests DAMP (Descriptive And Meaningful Phrases) - each test is now self-contained and readable without jumping to helper methods. Added descriptive constants (halfKbSize, kbSize) and variable names (singleUnitResult, midRangeResult, maxValueResult) to eliminate magic numbers and improve clarity. Fixed operator precedence for boundary value calculation - using 'baseValue * kbSize - 1' preserves correct semantics.
philipphofmann
force-pushed
the
test/aaa-pattern-damp-byte-count-formatter
branch
from
abb377a to
3c2da32
Compare
| private let halfKbSize: UInt = 512 | ||
| private let kbSize: UInt = 1_024 | ||
|
|
||
| func testBytesDescription() { |
There was a problem hiding this comment.
m: I feel like this is great opportunity to adopt our Test Naming Convention:
#### Test Naming Convention
Use the pattern `test<Function>_when<Condition>_should<Expected>()` for test method names:
**Format:** `test<Function>_when<Condition>_should<Expected>()`
**Examples:**
- ✅ `testAdd_whenSingleItem_shouldAppendToStorage()`
- ✅ `testAdd_whenMaxItemCountReached_shouldFlushImmediately()`
- ✅ `testCapture_whenEmptyBuffer_shouldDoNothing()`
- ✅ `testAdd_whenBeforeSendItemReturnsNil_shouldDropItem()`
**Benefits:**
- Clear function being tested
- Explicit condition/scenario
- Expected outcome is obvious
- Easy to understand test purpose without reading implementation
Applied Arrange-Act-Assert pattern with comment markers and made tests DAMP by inlining the helper method.
Changes
// -- Arrange --,// -- Act --,// -- Assert --comment markersassertDescriptionhelper to make each test self-containedmidRangeMultiplier,maxValueOffset)singleUnitResult,midRangeResult,maxValueResult)Rationale
DAMP tests are more readable and maintainable - each test can be understood independently without navigating to helper methods.
#skip-changelog
Closes #7222