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

Specify UTC timezone argument in datetime.now() and fix unit tests #6279

Merged
merged 7 commits into from
Sep 16, 2024

Conversation

onkar
Copy link
Member

@onkar onkar commented Sep 9, 2024

While running unit tests locally using make test, I saw these failures:

=========================================================================================================== short test summary info ===========================================================================================================
FAILED tests/clickhouse/optimize/test_optimize.py::test_optimize_partitions_raises_exception_with_cutoff_time - Failed: DID NOT RAISE <class 'snuba.clickhouse.optimize.optimize_scheduler.OptimizedSchedulerTimeout'>
FAILED tests/clickhouse/optimize/test_optimize_scheduler.py::test_get_next_schedule[non parallel] - assert OptimizationSchedule(partitions_groups=[["(90,'2022-06-27')",\n                                         "(90,'2022-06-20')",\n   ...
FAILED tests/clickhouse/optimize/test_optimize_scheduler.py::test_get_next_schedule[parallel before final cutoff] - assert OptimizationSchedule(partitions_groups=[["(90,'2022-03-28')",\n                                         "(90,'202...
FAILED tests/clickhouse/optimize/test_optimize_scheduler.py::test_get_next_schedule_raises_exception - Failed: DID NOT RAISE <class 'snuba.clickhouse.optimize.optimize_scheduler.OptimizedSchedulerTimeout'>
FAILED tests/subscriptions/test_scheduler_consumer.py::test_tick_time_shift - AssertionError: assert Tick(partition=0, offsets=Interval(lower=0, upper=1), timestamps=Interval(lower=86400.0, upper=172800.0)) == Tick(partition=0, offsets=...
========================================================================== 5 failed, 2601 passed, 4 skipped, 2 deselected, 1 xfailed, 6 xpassed in 430.69s (0:07:10) ==========================================================================

The reason for failure is that some of these tests use time_machine's context manager, which expects the time in UTC. When these tests are run locally in PST, there is a difference of 7 hours that never triggers the cutoff time expiry.

>>> from datetime import datetime, timezone
>>> datetime.now()
datetime.datetime(2024, 9, 9, 11, 51, 15, 279890)
>>> datetime.now(timezone.utc)
datetime.datetime(2024, 9, 9, 18, 51, 26, 40580, tzinfo=datetime.timezone.utc)

The fix is to use datetime's timezone.utc instead of datetime.now(). Datetime also used to have datetime.utcnow() but it has been deprecated.

Note: I also fixed some non-test code, so I would appreciate reviews from experts in those areas.

@onkar onkar requested a review from a team as a code owner September 9, 2024 18:56
@xurui-c
Copy link
Member

xurui-c commented Sep 9, 2024

I wonder if you can now remove the xfail here?

@onkar
Copy link
Member Author

onkar commented Sep 9, 2024

I wonder if you can now remove the xfail here?

Removed the xfail there and tests run successfully:

========================================================================= 2612 passed, 4 skipped, 2 deselected, 1 xfailed in 428.80s (0:07:08) ==========================================================================

Copy link

codecov bot commented Sep 9, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
662 1 661 1
View the top 1 failed tests by shortest run time
tests.clickhouse.optimize.test_optimize.TestOptimize test_optimize[errors non-parallel]
Stack Traces | 0.853s run time
Traceback (most recent call last):
  File ".../clickhouse/optimize/test_optimize.py", line 213, in test_optimize
    assert partitions == []
AssertionError: assert [Part(name="(90,'1999-11-15')", date=datetime.datetime(1999, 11, 15, 0, 0), retention_days=90)] == []
  Left contains one more item: Part(name="(90,'1999-11-15')", date=datetime.datetime(1999, 11, 15, 0, 0), retention_days=90)
  Full diff:
  - []
  + [Part(name="(90,'1999-11-15')", date=datetime.datetime(1999, 11, 15, 0, 0), retention_days=90)]

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@onkar
Copy link
Member Author

onkar commented Sep 9, 2024

I wonder if you can now remove the xfail here?

Removed the xfail there and tests run successfully:

========================================================================= 2612 passed, 4 skipped, 2 deselected, 1 xfailed in 428.80s (0:07:08) ==========================================================================

For some reason, the test_optimize.py tests fail in the CI but pass locally. I will take a look at this later. For now, I am going to keep the xfail as is and merge this PR.

@onkar onkar merged commit 7577534 into master Sep 16, 2024
30 checks passed
@onkar onkar deleted the onkar/unit-test-fix branch September 16, 2024 23:14
onkar added a commit that referenced this pull request Sep 17, 2024
onkar added a commit that referenced this pull request Sep 17, 2024
#6312)

…tests (#6279)"

This reverts commit 7577534.


Surprisingly, the commit runs fine locally and even in the CI but I am
seeing these failures:
-
https://github.com/getsentry/snuba/actions/runs/10893514288/job/30228865217
-
https://github.com/getsentry/snuba/actions/runs/10894057864/job/30230460999

Reverting this commit to unblock deploy-snuba-s4s.
onkar added a commit that referenced this pull request Sep 17, 2024
During the investigation of
#6279, I found that:
- `OPTIMIZE_JOB_CUTOFF_TIME` is overridden to `24` and it was causing
test failures.
- `OPTIMIZE_PARALLEL_MAX_JITTER_MINUTES` does not seem to be used
anywhere in the code.

With the change, less no. of local tests fail. The failure number is
non-zero because #6279 was
reverted yesterday . I would like to verify if this small change passes
through the CI and stays stable in prod.
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.

4 participants