Skip to content

[fix][test] Disable invalid test BrokerServiceTest#testBrokerStatsTopicLoadFailed#20494

Merged
lhotari merged 1 commit intoapache:masterfrom
lhotari:lh-disable-bad-test-in-BrokerServiceTest
Jun 5, 2023
Merged

[fix][test] Disable invalid test BrokerServiceTest#testBrokerStatsTopicLoadFailed#20494
lhotari merged 1 commit intoapache:masterfrom
lhotari:lh-disable-bad-test-in-BrokerServiceTest

Conversation

@lhotari
Copy link
Member

@lhotari lhotari commented Jun 5, 2023

Fixes #20492

Motivation

The test BrokerServiceTest#testBrokerStatsTopicLoadFailed introduced in #19236 is invalid.

  • It modifies the current shared state and that causes other tests to fail.
  • The test uses Awaitility with a 2 minute timeout. That is too long for unit tests.

Modifications

  • disable the test until it is fixed

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@lhotari lhotari self-assigned this Jun 5, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 5, 2023
@lhotari lhotari requested a review from Technoboy- June 5, 2023 13:35
@lhotari
Copy link
Member Author

lhotari commented Jun 5, 2023

/pulsarbot rerun-failure-checks

@lhotari lhotari requested a review from poorbarcode June 5, 2023 13:43
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

I suggest we either:

  1. Add a comment to the issue so that developers know where to find information on work for re-enable it (it can achieve by git blame, but why not?)
  2. Remove the test case and if the coverage is necessary, file a new ticket to add it back.

@lhotari
Copy link
Member Author

lhotari commented Jun 5, 2023

I suggest we either:

  1. Add a comment to the issue so that developers know where to find information on work for re-enable it (it can achieve by git blame, but why not?)
  2. Remove the test case and if the coverage is necessary, file a new ticket to add it back.

@tisonkun I'll create a ticket once this is merged. There's already a comment that it's disabled because it is flaky. I'm expecting that this is sufficient.

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Fair enough.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky-test: BrokerServiceTest.testConnectionController

5 participants