OBPIH-7158 Bump up unit tests coverage (0.9.4 test cycle)#5217
OBPIH-7158 Bump up unit tests coverage (0.9.4 test cycle)#5217awalkowiak merged 4 commits intodevelopfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5217 +/- ##
============================================
+ Coverage 8.19% 8.34% +0.15%
- Complexity 950 976 +26
============================================
Files 638 638
Lines 43214 43214
Branches 10503 10503
============================================
+ Hits 3542 3608 +66
+ Misses 39130 39051 -79
- Partials 542 555 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| given: | ||
| Set<CycleCountItem> cycleCountItems = new TreeSet<>() | ||
| for (int countIndex : countIndexes) { | ||
| CycleCountItem cycleCountItem = Spy(CycleCountItem) |
There was a problem hiding this comment.
Is the Spy and the TreeSet needed because otherwise the items are non-unique because they all have id==null?
If you wanted to avoid needing to mock/stub out the CycleCountItem so that you can test with real methods, you can do:
CycleCountItem item = new CycleCountItem(
countIndex: 1,
location: bin1,
status: CycleCountItemStatus.APPROVED
)
item.id = 1
CycleCount cycleCount = CycleCount cycleCount = new CycleCount(
cycleCountItems: [item]
)
Kind of weird to have to set the id manually, but that's the best solution that I've found
| void 'compareTo should return items in order starting with expiration date and location name'() { | ||
| given: | ||
| mockDomain(InventoryItem) | ||
| mockDomain(Location) |
There was a problem hiding this comment.
you can do mockDomains(InventoryItem, Location). But do you even need to mock the domains? I usually don't need to unless I call .save() on the domain
| cycleCountItem2.location.name = 'A' | ||
| cycleCountItem3.location.name = 'B' | ||
| cycleCountItem4.location.name = 'C' | ||
| cycleCountItem5.location.name = 'C' |
There was a problem hiding this comment.
This is fine, but I find it clearer to declare the fields in the constructor:
CycleCountItem cycleCountItem1 = new CycleCountItem(
inventoryItem: new InventoryItem(
expirationDate: new Date(2025, 03, 02),
),
location: new Location(
name: 'A',
),
)
...
| cycleCountItems.forEach { println(it.hashCode()) } | ||
| println("====") | ||
| cycleCountItems.sort() | ||
| cycleCountItems.forEach { println(it.hashCode()) } |
There was a problem hiding this comment.
also can't you do?
List<CycleCountItem> cycleCountItems = [cycleCountItem5, cycleCountItem4, cycleCountItem3, cycleCountItem2, cycleCountItem1];
| CycleCountRequestCommand cycleCountRequestCommand = new CycleCountRequestCommand( | ||
| product: Spy(Product) { | ||
| getProductCode() >> PRODUCT_CODE | ||
| }, |
There was a problem hiding this comment.
why Spy instead of new Product(productCode: PRODUCT_CODE)?
| } | ||
|
|
||
| when: "the command is invalid" | ||
| cycleCountRequestBatchCommand.validate() |
There was a problem hiding this comment.
I don't think asserts like this register under a "when" block. Also shouldn't it be false since it fails validation? You'd want:
...
then:
assert !cycleCountRequestBatchCommand.validate()
when:
controller.createRequests(cycleCountRequestBatchCommand)
then:
thrown(ValidationException)
(I like to explicitly declare my asserts with "assert" in tests.)
There was a problem hiding this comment.
I didn't treat this as an assertion - after calling the validate, the errors are assigned to CycleCountRequestCommand, so that the errors are thrown in createRequests
| cycleCountItem.countIndex = countIndex | ||
| cycleCountItems.add(cycleCountItem) | ||
| } | ||
| CycleCount cycleCount = Spy(CycleCount) |
There was a problem hiding this comment.
if you're not using Spy anymore in the above test, probably should be consistent and not use it here either
3 participants
I am pushing my current version of unit tests because I don't have more available time to add a few more