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

Add missing test cases for next()/previous() #5777

Merged
merged 4 commits into from
Nov 2, 2023

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Oct 19, 2023

We were not testing the case of next() and previous() where the truncated properties were the same as the current datetime. E.g.

>>> now = 2020-10-19T13:53Z

>>> next(--10-19)
2020-10-19T00:00Z

>>> previous(--10-19)
2019-10-19T00:00Z

This behaviour does not seem to have been rigorously defined before? Adding these test cases ensures we do not break compatibility

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • No dependency changes
  • Tests are included
  • No changelog entry needed
  • No docs needed
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@wxtim
Copy link
Member

wxtim commented Oct 20, 2023

It strikes me that we also ought to test for

>>> now = 2020-10-19T13:53Z

>>> next(T-53)
2020-10-19T13:53Z

>>> previous(T-53)
2019-10-19T12:53Z

Although in the latter case I'm really not clear what I'd expect the user to expect the correct answer to be. I think it probably should be the previous hour.

@MetRonnie
Copy link
Member Author

MetRonnie commented Oct 20, 2023

Hang on a minute, it strikes me that

>>> now = 2020-10-19T13:53Z
>>> next(--10-19)
2020-10-19T00:00Z

is a bug Something is wrong :( because it returns a datetime that is in the past!

It should either be

2021-10-19T00:00Z  # next year

or

2020-10-19T13:53Z  # same datetime

@wxtim
Copy link
Member

wxtim commented Oct 20, 2023

is a bug because it returns a datetime that is in the past!

That's definitely a bug. 🐞 ← (a well spotted bug)

@MetRonnie MetRonnie added question Flag this as a question for the next Cylc project meeting. bug? Not sure if this is a bug or not bug Something is wrong :( and removed bug? Not sure if this is a bug or not labels Oct 20, 2023
@oliver-sanders
Copy link
Member

Yes, I would expect next to return a time in the future too.

@MetRonnie
Copy link
Member Author

MetRonnie commented Oct 27, 2023

So the question Flag this as a question for the next Cylc project meeting. is, which of these should be the behaviour?

2021-10-19T00:00Z  # next year

or

2020-10-19T13:53Z  # same datetime

@MetRonnie MetRonnie marked this pull request as draft October 27, 2023 11:13
@oliver-sanders
Copy link
Member

oliver-sanders commented Oct 27, 2023

The one that's in the future ("next year") because just as T00 is short for T0000 and 01 is short for 01T0000, --10-19 is short for --10-19T0000.

I.E. matching date-time components should not be a special case.

@MetRonnie MetRonnie removed bug Something is wrong :( question Flag this as a question for the next Cylc project meeting. labels Oct 27, 2023
@MetRonnie MetRonnie marked this pull request as ready for review October 27, 2023 14:52
@MetRonnie MetRonnie requested a review from wxtim October 27, 2023 15:03
@oliver-sanders oliver-sanders modified the milestones: cylc-8.2.3, cylc-8.2.4 Nov 1, 2023
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

Nice use of xfail. Worth putting a comment with a link to #5795 in the code nearby?

@oliver-sanders oliver-sanders modified the milestones: cylc-8.2.4, cylc-8.2.3 Nov 2, 2023
@oliver-sanders oliver-sanders merged commit 238ce28 into cylc:8.2.x Nov 2, 2023
@MetRonnie MetRonnie deleted the testing branch November 2, 2023 11:55
@MetRonnie
Copy link
Member Author

Worth putting a comment with a link to #5795 in the code nearby?

A curious dev can use git blame

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

Successfully merging this pull request may close these issues.

3 participants