Fix flaky test_isExpired_when_current_date_then_returns_true
#16074
Conversation
| import Foundation | ||
| import WooFoundation | ||
| import struct Alamofire.JSONEncoding | ||
| import struct NetworkingCore.JetpackSite |
There was a problem hiding this comment.
I cannot compile the test target unless I add this import, which seems to come from a missing rake generate somewhere else.
There was a problem hiding this comment.
The latest commit in trunk doesn't have this import and passed CI though, could it be from packages not being fully resolved / a need to build the app before tests / a need for a clean build?
There was a problem hiding this comment.
I tried removing this line, building the app then ran POSCouponTests, both the app and tests ran. I've had frustrating times when it took a few tries rebuilding/cleaning though. If you get to build and run tests without this line, let's remove this.
iamgabrielma
added
type: task
|
|
jaclync
left a comment
jaclync
left a comment
There was a problem hiding this comment.
Left some comments, lemme know what you think!
| @Test func test_isExpired_when_current_date_then_returns_true() { | ||
| // Given | ||
| let now = Date() | ||
| let now = fixedDate(daysFromReference: 0) |
There was a problem hiding this comment.
Using a previous date for dateExpires while isExpired is comparing against Date() makes it not exactly fit the goal of the test case test_isExpired_when_current_date_then_returns_true, which is when the expiration date is the same as the current date.
A common approach is to mock the Date() in the code we're testing. e.g. from the existing POSCoupon.isExpired:
to be a function that takes in currentDate: Date = Date():
public func isExpired(currentDate: Date = Date()) -> Bool {
guard let dateExpires else {
return false
}
return dateExpires < currentDate
}This way, we can pass any date values for the current date and expiration date.
Also, from the logic, wouldn't isExpired return false when the current date and dateExpires are the same? 🤔 Like it expires right on the expiration date, which sounds like the expected behavior.
There was a problem hiding this comment.
This way, we can pass any date values for the current date and expiration date.
Yes, so my point with "As a 3rd option, we could change the details of POSCoupon, but I don't see a good reason to change the interface only because of tests." is:
If we would change the isExpired computed var to be a function that might expect a date to be passed in there should be a reason that is not testing only, right? Since we never pass any date in the implementation, nor is expected, my first gut check was that it shouldn't be a function neither accept a date as param, as this change to the implementation would be only for the sake of tests. So should we actually make the change?
I'm happy with making the change if you feel is worth it, just questioning in case this comes up again on a different place with a similar shape 🤔 otherwise might be better to just remove it.
There was a problem hiding this comment.
👋 I hadn't seen this PR, but fixed this in a simpler way...
WDYT about just making it return dateExpires <= Date()? That makes it match the existing tests without changing any interfaces.
There was a problem hiding this comment.
Thanks Josh! Let's go ahead with your PR, its more correct to treat them as expired as soon as the date is equal to expiration, fixing the flakiness as well. I'll mark this one as closed.
| import Foundation | ||
| import WooFoundation | ||
| import struct Alamofire.JSONEncoding | ||
| import struct NetworkingCore.JetpackSite |
There was a problem hiding this comment.
The latest commit in trunk doesn't have this import and passed CI though, could it be from packages not being fully resolved / a need to build the app before tests / a need for a clean build?
| import Foundation | ||
| import WooFoundation | ||
| import struct Alamofire.JSONEncoding | ||
| import struct NetworkingCore.JetpackSite |
There was a problem hiding this comment.
I tried removing this line, building the app then ran POSCouponTests, both the app and tests ran. I've had frustrating times when it took a few tries rebuilding/cleaning though. If you get to build and run tests without this line, let's remove this.
Yeah, for some reason it took a couple of clean/rebuilds to make it work 😅, I'll revert it. |
Closes WOOMOB-1255
Description
This PR mocks the
nowdate to a fixed date so we avoid flakiness ontest_isExpired_when_current_date_then_returns_true.I'm not convinced of the value of the test, since in the implementation of POSCoupon we check the expiration against
Date(), so a 2nd option could be to just pass "now" as "a few seconds ago" or similar, which should be closer to the real implementation.We cannot use the fixed date either in the tests that check for future dates, otherwise sooner or later the fixed date passed in, will not surpass the actual date of comparison, failing the test (ie: future date as tomorrow in 2024, is still <
Date()).As a 3rd option, we could change the details of POSCoupon, but I don't see a good reason to change the interface only because of tests.
Let me know what you think, I might lean to option 2 instead, where "now" is actually a couple of seconds ago, rather than using a fixed date.
Testing information