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 off-by-one error #4822

Closed
wants to merge 1 commit into from
Closed

fix off-by-one error #4822

wants to merge 1 commit into from

Conversation

dorner
Copy link
Collaborator

@dorner dorner commented Dec 2, 2024

No description provided.

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hey @dorner -- When I run this, a couple of the tests in distributions_by_county_system_spec.rb fail, but when I run them in main they pass. Could you elaborate on what this change is about?

@coalest
Copy link
Collaborator

coalest commented Dec 2, 2024

@cielf A PR of mine was just merged and it added these flaky tests. They test the new filtering by "Prior Year" or "Last 12 Months" options in the date range picker. They create distributions right before and right after the date range filter. But currently there is some overlap based on time so the tests are flaky.

@cielf
Copy link
Collaborator

cielf commented Dec 3, 2024

So, my understanding is that this fails dependent on the time of day it is run, then.

@coalest
Copy link
Collaborator

coalest commented Dec 4, 2024

That's what I thought at first, but the fact that one of the specs is still flaky with this PR (that sets an artificial date and time), makes me think there is something additional wrong. My theory is it's also time zone related between UTC vs the time zone set in the test environment (US west coast).

I think a combination of this PR plus the one I just created will fix the issue.

@dorner
Copy link
Collaborator Author

dorner commented Dec 10, 2024

This was a failed attempt at fixing the flaky test. @coalest fixed it in a different PR.

@dorner dorner closed this Dec 10, 2024
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.

3 participants