Skip to content

Speed up 10x test_storage_rabbitmq::test_failed_connection#79217

Merged
pamarcos merged 5 commits intomasterfrom
make-test_storage_rabbitmq-faster
Apr 15, 2025
Merged

Speed up 10x test_storage_rabbitmq::test_failed_connection#79217
pamarcos merged 5 commits intomasterfrom
make-test_storage_rabbitmq-faster

Conversation

@pamarcos
Copy link
Copy Markdown
Member

@pamarcos pamarcos commented Apr 15, 2025

This is the first thing I should've done to make my life easier when testing it. Now that I have grasped better what the test does, I can do it in confidence:

Tune the numbers to get the same behavior but with much less work done. This is not an stress test, so it's still good if we use 100 messages instead of several thousands as long as we test the same thing.

Before

91.13s call     test_storage_rabbitmq/test_failed_connection.py::test_rabbitmq_restore_failed_connection_without_losses_1
87.11s call     test_storage_rabbitmq/test_failed_connection.py::test_rabbitmq_restore_failed_connection_without_losses_2
8.56s teardown test_storage_rabbitmq/test_failed_connection.py::test_rabbitmq_restore_failed_connection_without_losses_2
6.23s setup    test_storage_rabbitmq/test_failed_connection.py::test_rabbitmq_restore_failed_connection_without_losses_1
3.22s teardown test_storage_rabbitmq/test_failed_connection.py::test_rabbitmq_restore_failed_connection_without_losses_1
0.45s setup    test_storage_rabbitmq/test_failed_connection.py::test_rabbitmq_restore_failed_connection_without_losses_2

After

14.35s call     test_storage_rabbitmq/test_failed_connection.py::test_rabbitmq_restore_failed_connection_without_losses_1
13.54s call     test_storage_rabbitmq/test_failed_connection.py::test_rabbitmq_restore_failed_connection_without_losses_2
8.32s teardown test_storage_rabbitmq/test_failed_connection.py::test_rabbitmq_restore_failed_connection_without_losses_2
6.35s setup    test_storage_rabbitmq/test_failed_connection.py::test_rabbitmq_restore_failed_connection_without_losses_1
3.42s teardown test_storage_rabbitmq/test_failed_connection.py::test_rabbitmq_restore_failed_connection_without_losses_1
0.40s setup    test_storage_rabbitmq/test_failed_connection.py::test_rabbitmq_restore_failed_connection_without_losses_2

Also, the RabitMQMonitor seems not very consistent according to some false positive reports such as https://s3.amazonaws.com/clickhouse-test-reports/PRs/79212/460d6f15bdb3b12c485c171a7e13c9bb7955e4a6//integration_tests_asan_old_analyzer_1_6/integration_run_test_storage_rabbitmq_test_failed_connection_py_0.log

We're getting this, even though all messages were consumed ok in ClickHouse:

        if self.expected_published > 0 and self.expected_published != len(self.published):
>           pytest.fail(f"{len(self.published)}/{self.expected_published} (got/expected) messages published. Sample of not published: {_get_non_present(self.published, self.expected_published)}")
E           Failed: 195056/300000 (got/expected) messages published. Sample of not published: [195056, 195057, 195058, 195059, 195060, 195061, 195062, 195063, 195064, 195065]

Thus, let's make it a warning log instead of a hard failure for now

Related to #71049

Changelog category (leave one):

  • CI Fix or Improvement (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

...

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Tune the numbers to get the same behavior but with much less
work done. This is not an stress test, so it's still good
if we use 1000 messages instead of several thousands as long
as we test the same thing.
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Apr 15, 2025

Workflow [PR], commit [3df7dbb]

@clickhouse-gh clickhouse-gh Bot added the pr-ci label Apr 15, 2025
@pamarcos pamarcos added 🍃 green ci 🌿 Fixing flaky tests in CI ci-integration-test CI with integration test jobs only labels Apr 15, 2025
@pamarcos pamarcos force-pushed the make-test_storage_rabbitmq-faster branch from 52bb0e8 to e502595 Compare April 15, 2025 15:17
@pamarcos pamarcos added ci-integration-test-flaky Run only integration flaky tests and removed ci-integration-test CI with integration test jobs only labels Apr 15, 2025
@pamarcos pamarcos requested a review from kssenii April 15, 2025 15:38
Comment thread tests/integration/test_storage_rabbitmq/test_failed_connection.py
@pamarcos pamarcos force-pushed the make-test_storage_rabbitmq-faster branch from a0ab240 to 3df7dbb Compare April 15, 2025 16:52
@pamarcos pamarcos enabled auto-merge April 15, 2025 17:01
@pamarcos pamarcos added this pull request to the merge queue Apr 15, 2025
Merged via the queue into master with commit 865cf34 Apr 15, 2025
117 of 119 checks passed
@pamarcos pamarcos deleted the make-test_storage_rabbitmq-faster branch April 15, 2025 17:07
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-integration-test-flaky Run only integration flaky tests 🍃 green ci 🌿 Fixing flaky tests in CI pr-ci pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants