Skip to content
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.

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

Fix upper bound in Calendar.RecurrenceRule.recurrences(of:in:) #1041

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Salynn
Copy link

@Salynn Salynn commented Nov 9, 2024

This commit fixes the use of the in: bound in
Calendar.RecurrenceRule.recurrences(of:in:) to properly exclude the upper bound as revealed by #1040.

This commit fixes the use of the `in:` bound in
Calendar.RecurrenceRule.recurrences(of:in:) to properly exclude the
upper bound as revealed by swiftlang#1040.
@Salynn
Copy link
Author

Salynn commented Nov 9, 2024

@swift-ci please test

let eventStart = Date(timeIntervalSince1970: 1285077600.0) // 2010-09-21T14:00:00-0000
let results = Array(rule.recurrences(of: eventStart, in: start..<end))

XCTAssertNotEqual(results.last!, end)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing XCTAssertNotEqual, can you XCTAssertEqual what the last result should be?

Copy link
Author

Choose a reason for hiding this comment

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

I can, but that changes the intent of the test, which is to ensure that end is not part of the recurrence. What the last value is doesn't actually matter for the purpose of this test.

Copy link
Author

@Salynn Salynn Nov 9, 2024

Choose a reason for hiding this comment

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

How about changing the assertion to XCTAssertLessThan?

Copy link
Contributor

@itingliu itingliu Nov 11, 2024

Choose a reason for hiding this comment

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

I understand what you want to test, but this line, XCTAssertNotEqual(results.last!, end), alone can mean anything if I hadn't looked at the code change. For example, this assert would pass if results is just an arbitrary value. Same goes with XCTAssertLessThan.

Also, instead of force unwrapping results.last!, it's useful to write it as XCTUnwrap (and have this test throw) instead of just force unwrapping it so we get a nice test report if it fails

Copy link
Author

Choose a reason for hiding this comment

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

I was unaware of XCTUnwrap! That's neat! I'll make the changes later today!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants