Skip to content

Fix test_dictionaries_update_and_reload::test_reload_after_fail_by_timer flakiness#67754

Closed
azat wants to merge 1 commit intoClickHouse:masterfrom
azat:tests/test_dictionaries_update_and_reload
Closed

Fix test_dictionaries_update_and_reload::test_reload_after_fail_by_timer flakiness#67754
azat wants to merge 1 commit intoClickHouse:masterfrom
azat:tests/test_dictionaries_update_and_reload

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented Aug 4, 2024

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

@azat
Fix test_dictionaries_update_and_reload::test_reload_after_fail_by_ti…
e6921bf 
…mer flakiness

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
This was referenced Aug 4, 2024
Merged
Merged
@azat azat added the 🍃 green ci 🌿 Fixing flaky tests in CI label Aug 4, 2024
@robot-ch-test-poll robot-ch-test-poll added the pr-not-for-changelog This PR should not be mentioned in the changelog label Aug 4, 2024
@alexey-milovidov alexey-milovidov self-assigned this Aug 4, 2024
@robot-ch-test-poll2
Copy link
Copy Markdown
Contributor

robot-ch-test-poll2 commented Aug 4, 2024 •
edited by robot-ch-test-poll3
Loading

This is an automated comment for commit e6921bf with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check nameDescriptionStatus
Flaky testsChecks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integration tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc❌ failure
Successful checks
Check nameDescriptionStatus
BuildsThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS✅ success
Docs checkBuilds and tests the documentation✅ success
Fast testNormally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here✅ success
Integration testsThe integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests✅ success
Style checkRuns a set of checks to keep the code style clean. If some of tests failed, see the related log from the report✅ success
Unit testsRuns the unit tests for different release types✅ success
Upgrade checkRuns stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts✅ success

@fm4v
Copy link
Copy Markdown
Member

fm4v commented Aug 4, 2024

I don't like this change because now we expect both statuses ["FAILED", "FAILED_AND_RELOADING"] even in release build. Original fix expect only "FAIL" status and in sanitizer builds "FAILED_AND_RELOADING" is expected for some time which is always changed to "FAILED".
I think it's better to increase retry_count and add same retries for second assert.

Anyway this code is wrong, since you still can get FAILED_AND_RELOADING two lines below, there should be else at least.

Original changes fixes problem in first assert. I didn't see fix for the second assert in this PR too.

According to fails for the last 90 days:
3 fails in second asserts:
1 2 3
2 fails in first assert:
1 2

Also flakiness rate is decreased after first fix and why it happens?

@fm4v fm4v self-requested a review August 4, 2024 16:30
@azat
Copy link
Copy Markdown
Member Author

azat commented Aug 4, 2024 •
edited
Loading

I don't like this change because now we expect both statuses ["FAILED", "FAILED_AND_RELOADING"] even in release build

@fm4v

  • Why it is possible to have FAILED_AND_RELOADING under sanitizers but not release build? - my guess is that the sanitiazers builds are just slow (see comments), if so, then it may fail under release as well, and it is just a matter of time when it happens...
  • Why allowing FAILED_AND_RELOADING is bad? - personally I don't see any issue with this

@azat
Copy link
Copy Markdown
Member Author

azat commented Aug 4, 2024

This test is not idempotent and should be rewritten completely

@azat
Copy link
Copy Markdown
Member Author

azat commented Aug 4, 2024

@fm4v help is welcome

@fm4v
Copy link
Copy Markdown
Member

fm4v commented Aug 5, 2024

Close in favor of #67793

@fm4v fm4v closed this Aug 5, 2024
@qoega
Copy link
Copy Markdown
Member

qoega commented Aug 5, 2024 •
edited
Loading

This test is not idempotent and should be rewritten completely

+1 to this. Trivial ways to fix is to either drop all dictionaries in the end of a test or to have unique dictionary name in every test run. If we rely on logs, make it dependent on specific test run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

@fm4v fm4v Awaiting requested review from fm4v

Assignees

@alexey-milovidov alexey-milovidov

Labels

🍃 green ci 🌿 Fixing flaky tests in CI pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@azat @robot-ch-test-poll2 @fm4v @qoega @alexey-milovidov @robot-ch-test-poll