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

SNOW-1357844: fix flaky test caused by oob telemetry datetime comparsion #1513

Conversation

sfc-gh-aling
Copy link
Contributor

@sfc-gh-aling sfc-gh-aling commented May 3, 2024

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-1357844

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

pytest.approx only supports number, datetime is not supported, so previously implementation doesn't work at all.
change to use timestamp to the approx equal

@sfc-gh-aling sfc-gh-aling added NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md local testing Local Testing issues/PRs labels May 3, 2024
@sfc-gh-aling sfc-gh-aling requested a review from a team as a code owner May 3, 2024 21:38
@sfc-gh-mvashishtha
Copy link
Contributor

@sfc-gh-aling could you please pip install pytest-repeat pytest-xdist and run the test 1000 times before and after this change (I think parallelism with -n 32 might help speed it up) to check that the flakiness goes away?

@sfc-gh-aling
Copy link
Contributor Author

sfc-gh-aling commented May 3, 2024

@sfc-gh-mvashishtha I changed to use mock, we shall be good now. thanks for the suggestion

command pytest tests/mock_unit/test_oob_telemetry.py::test_unit_oob_log_not_implemented_error --count 1000 -n 32 passes

Copy link
Contributor

@sfc-gh-mvashishtha sfc-gh-mvashishtha left a comment

Choose a reason for hiding this comment

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

@sfc-gh-aling one more nit

command pytest tests/mock_unit/test_oob_telemetry.py::test_unit_oob_log_not_implemented_error --count 1000 -n 32 passes

Thanks for checking, but it seems to pass on the main branch as well, so it doesn't reproduce the flake. That's okay. I think mocking the time should work.

tests/mock_unit/test_oob_telemetry.py Outdated Show resolved Hide resolved
Copy link
Contributor

@sfc-gh-mvashishtha sfc-gh-mvashishtha left a comment

Choose a reason for hiding this comment

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

LGTM! thanks

@sfc-gh-aling sfc-gh-aling merged commit 561340d into main May 6, 2024
25 checks passed
@sfc-gh-aling sfc-gh-aling deleted the SNOW-1357844-fix-flaky-local-tests-mock-unit-test-oob-telemetry-py-test-unit-oob-log-not-implemented-error branch May 6, 2024 21:40
@github-actions github-actions bot locked and limited conversation to collaborators May 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
local testing Local Testing issues/PRs NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants