Skip to content

Fix flaky test_isExpired_when_current_date_then_returns_true #16074

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Fix flaky test_isExpired_when_current_date_then_returns_true #16074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca1695f
update test for expired-now
iamgabrielma Sep 3, 2025
5d92305
test target not building due missing codegen
iamgabrielma Sep 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Modules/Sources/Networking/Model/Copiable/Models+Copiable.generated.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import Codegen
import Foundation
import WooFoundation
import struct Alamofire.JSONEncoding
import struct NetworkingCore.JetpackSite
Copy link
Contributor Author

@iamgabrielma iamgabrielma Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot compile the test target unless I add this import, which seems to come from a missing rake generate somewhere else.

Copy link
Contributor

@jaclync jaclync Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@jaclync jaclync Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



extension Networking.AIProduct {
Expand Down
26 changes: 25 additions & 1 deletion Modules/Tests/YosemiteTests/PointOfSale/POSCouponTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,34 @@ struct POSCouponTests {

@Test func test_isExpired_when_current_date_then_returns_true() {
// Given
let now = Date()
let now = fixedDate(daysFromReference: 0)
Copy link
Contributor

@jaclync jaclync Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

woocommerce-ios/Modules/Sources/Yosemite/PointOfSale/Coupons/POSCoupon.swift

Lines 16 to 21 in fa8489e

public var isExpired: Bool {
guard let dateExpires = dateExpires else {
return false
}
return dateExpires < Date()
}

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.

Copy link
Contributor Author

@iamgabrielma iamgabrielma Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@joshheald joshheald Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 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.

Copy link
Contributor Author

@iamgabrielma iamgabrielma Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let coupon = POSCoupon(id: UUID(), code: "expired-now", dateExpires: now)

// Then
#expect(coupon.isExpired == true, "A coupon expiring at the current time should be considered expired")
}
}

private extension POSCouponTests {
/// Returns a fixed date with optional day offset
/// - Parameter daysFromReference: Number of days to add/subtract from the reference date (0 = reference date)
/// - Returns: A fixed date
func fixedDate(daysFromReference: Int = 0) -> Date {
var components = DateComponents()
components.year = 2024
components.month = 6
components.day = 15
components.hour = 12
components.minute = 0
components.second = 0
components.timeZone = TimeZone(identifier: "UTC")

guard let baseDate = Calendar(identifier: .gregorian).date(from: components) else {
// Fallback to the same date (2024-06-15 12:00:00 UTC) if calendar creation fails
return Date(timeIntervalSince1970: 1718452800)
}
guard daysFromReference != 0 else { return baseDate }

return Calendar.current.date(byAdding: .day, value: daysFromReference, to: baseDate) ?? baseDate
}
}
Loading