Skip to content

Disable test_overcommit_tracker for llvm-coverage#101381

Merged
nickitat merged 1 commit intomasterfrom
nickitat-patch-35
Apr 1, 2026
Merged

Disable test_overcommit_tracker for llvm-coverage#101381
nickitat merged 1 commit intomasterfrom
nickitat-patch-35

Conversation

@nickitat
Copy link
Copy Markdown
Member

@nickitat nickitat commented Mar 31, 2026

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

cidb

@nickitat
Disable test_overcommit_tracker for llvm-coverage
8ec2493 
https://play.clickhouse.com/play?user=play&run=1#V0lUSAogICAgOTAgQVMgaW50ZXJ2YWxfZGF5cwpTRUxFQ1QKICAgIHRvU3RhcnRPZkRheShjaGVja19zdGFydF90aW1lKSBBUyBkYXksCiAgICBjaGVja19uYW1lLAogICAgY291bnQoKSBBUyBmYWlsdXJlcywKICAgIGdyb3VwVW5pcUFycmF5KHB1bGxfcmVxdWVzdF9udW1iZXIpIEFTIHBycywKICAgIGdyb3VwVW5pcUFycmF5SWYocmVwb3J0X3VybCwgcmVwb3J0X3VybCBOT1QgTElLRSAnJWNvdmVyYWdlJScpIEFTIHJlcG9ydF91cmwKRlJPTSBjaGVja3MKV0hFUkUgKG5vdygpIC0gdG9JbnRlcnZhbERheShpbnRlcnZhbF9kYXlzKSkgPD0gY2hlY2tfc3RhcnRfdGltZQogICAgQU5EIHRlc3RfbmFtZSA9ICd0ZXN0X292ZXJjb21taXRfdHJhY2tlci90ZXN0LnB5Ojp0ZXN0X3VzZXJfb3ZlcmNvbW1pdCcKICAgIC0tIEFORCBjaGVja19uYW1lID0gJ0ludGVncmF0aW9uIHRlc3RzIChhbWRfbGx2bV9jb3ZlcmFnZSwgMi81KScKICAgIEFORCB0ZXN0X3N0YXR1cyBJTiAoJ0ZBSUwnLCAnRVJST1InKQogICAgQU5EIChwdWxsX3JlcXVlc3RfbnVtYmVyID0gMCBPUiBiYXNlX3JlZiBJTiAoJ21hc3RlcicpKQogICAgQU5EIHRlc3RfY29udGV4dF9yYXcgTElLRSAnJUFzc2VydGlvbkVycm9yJScKR1JPVVAgQlkgZGF5LCBjaGVja19uYW1lCk9SREVSIEJZIGRheSBERVNDLCBjaGVja19uYW1lCg==
@nickitat nickitat linked an issue Mar 31, 2026 that may be closed by this pull request
Flaky test: test_overcommit_tracker #101318
Open
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 31, 2026 •
edited by nickitat
Loading

Workflow [PR], commit [8ec2493]

Summary: ❌

job_name test_name status info comment
Integration tests (amd_asan_ubsan, db disk, old analyzer, 2/6) failure
test_backup_restore_s3/test.py::test_backup_to_s3_different_credentials[data_file_name_from_first_file_name-native_multipart] FAIL cidb, issue ISSUE EXISTS

AI Review

Summary

This PR updates test_overcommit_tracker to skip under LLVM coverage in addition to ASan/MSan, which matches the linked flaky-failure context. The code change is correct and low risk. I found one minor issue in the skip message text: it still mentions only sanitizers, which is now slightly misleading in logs.

Findings

💡 Nits

  • [tests/integration/test_overcommit_tracker/test.py:32] The skip reason says "under sanitizers" while the condition also skips for node.is_built_with_llvm_coverage(). This can confuse triage when reading CI logs.
    • Suggested fix: update the message to mention LLVM coverage (for example, "under sanitizers or LLVM coverage").
ClickHouse Rules
Item Status Notes
Deletion logging ➖
Serialization versioning ➖
Core-area scrutiny ➖
No test removal ✅
Experimental gate ➖
No magic constants ✅
Backward compatibility ✅
SettingsChangesHistory.cpp ➖
PR metadata quality ✅
Safe rollout ✅
Compilation time ✅
Final Verdict
  • Status: ⚠️ Request changes
  • Minimum required actions:
    • Update the skip message to include LLVM coverage so the logged reason matches the condition.

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Mar 31, 2026
clickhouse-gh[bot]
clickhouse-gh bot reviewed Mar 31, 2026
View reviewed changes
tests/integration/test_overcommit_tracker/test.py
if node.is_built_with_memory_sanitizer() or node.is_built_with_address_sanitizer():
if node.is_built_with_memory_sanitizer() or node.is_built_with_address_sanitizer() or node.is_built_with_llvm_coverage():
pytest.skip(
"doesn't fit in memory limits under sanitizers (memory overhead causes timeouts)"
Copy link
Copy Markdown
Contributor

@clickhouse-gh clickhouse-gh bot Mar 31, 2026

Choose a reason for hiding this comment

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

The skip condition now includes node.is_built_with_llvm_coverage(), but the skip message still says "under sanitizers". Could you update the message to mention LLVM coverage too (for example, "under sanitizers or LLVM coverage") so the reason is accurate in test logs?

@nickitat
Copy link
Copy Markdown
Member Author

nickitat commented Mar 31, 2026

@azat, maybe it makes sense to disable it first, then play with the settings in the background. up to you.

azat
azat approved these changes Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@azat azat left a comment

Choose a reason for hiding this comment

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

If you think it make sense to merge this first, feel free to do so

@nickitat nickitat enabled auto-merge April 1, 2026 13:26
@nickitat nickitat added this pull request to the merge queue Apr 1, 2026
Merged via the queue into master with commit a43bd66 Apr 1, 2026
162 of 164 checks passed
@nickitat nickitat deleted the nickitat-patch-35 branch April 1, 2026 13:37
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 1, 2026
This was referenced Apr 1, 2026
Merged
Merged
@vdimir vdimir mentioned this pull request Apr 1, 2026
Open
1 task
@tuanpach tuanpach mentioned this pull request Apr 2, 2026
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clickhouse-gh clickhouse-gh[bot] clickhouse-gh[bot] left review comments

@azat azat azat approved these changes

Assignees

@azat azat

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Flaky test: test_overcommit_tracker

3 participants

@nickitat @azat @robot-clickhouse