[fix][test] Fix flaky test ExtensibleLoadManagerTest.testIsolationPolicy#20609
Conversation
tisonkun
left a comment
There was a problem hiding this comment.
Can you provide a bit of details about why we should match this extra pattern? I suppose it is what the failure test throws but it can be either the test is wrong or the implementation is wrong.
One nit style comment inline.
| } catch (Exception ex) { | ||
| log.error("Failed to lookup topic: ", ex); | ||
| assertTrue(ex.getMessage().contains("Failed to look up a broker")); | ||
| assertTrue(ex.getMessage().contains("Failed to look up a broker") |
There was a problem hiding this comment.
Perhaps you can try to use assertj to assert contains so that we know the message instead of "expect true, actual false".
There was a problem hiding this comment.
Makes sense. I didn't realize that we already have AssertJ as a dependency since it's rarely used in the Pulsar code base.
There was a problem hiding this comment.
@tisonkun I switched to use AssertJ. However, there was a need to upgrade the AssertJ version since .containsAnyOf wasn't available in the version that was used. That is available since AssertJ 3.21.0, we were using 3.18.1 . I upgraded to latest AssertJ version which is 3.24.2 .
There was a problem hiding this comment.
Thank you! I'm not faimilar with this code so I cannot give an approval without more details. But @BewareMyPower already approves so it can be fine, with his endorsement :D
@tisonkun This test is failing in very many cases so this is really flaky and blocking CI. |
| } catch (Exception ex) { | ||
| log.error("Failed to lookup topic: ", ex); | ||
| assertTrue(ex.getMessage().contains("Failed to look up a broker")); | ||
| assertThat(ex.getMessage()).containsAnyOf("Failed to look up a broker", |
There was a problem hiding this comment.
I think it should always fail as "Failed to select the new owner broker for bundle".
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #20609 +/- ##
=============================================
+ Coverage 31.87% 73.07% +41.19%
- Complexity 11759 32010 +20251
=============================================
Files 1498 1867 +369
Lines 114593 138660 +24067
Branches 12425 15237 +2812
=============================================
+ Hits 36532 101323 +64791
+ Misses 73215 29318 -43897
- Partials 4846 8019 +3173
Flags with carried forward coverage won't be shown. Click here to find out more. |
Fixes #20608
Motivation
ExtensibleLoadManagerTest.testIsolationPolicy is flaky
Modifications
Documentation
docdoc-requireddoc-not-neededdoc-complete