Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http-netty: Disable the ClientEffectiveStrategyTest suite #3075

Conversation

bryce-anderson
Copy link
Contributor

@bryce-anderson bryce-anderson commented Oct 8, 2024

Motivation:

The ClientEffectiveStrategyTest suite has been flaky for a long time and multiple engineers have looked at it, without making progress. In the meantime, this suite causes a substantial fraction of the failed runs.

Modifications:

Mark the flaky test as @Disabled.

Result:

CI should be significantly less flaky.

Related to #2245.

Motivation:

The ClientEffectiveStrategyTest suite has been flaky for a long time
and multiple engineers have looked at it, without making progress.
In the meantime, this suite causes a substantial fraction of the
failed runs.

Modifications:

Mark the flaky test as @ignored.

Result:

CI should be significantly less flaky.
@bryce-anderson bryce-anderson merged commit c203ed2 into apple:main Oct 11, 2024
11 checks passed
@bryce-anderson bryce-anderson deleted the bl_anderson/disable-ClientEffectiveStrategyTest branch October 11, 2024 15:14
@@ -227,6 +228,7 @@ static Stream<Arguments> casesSupplier() {
return arguments.stream();
}

@Disabled // Disabled due to continued flakiness. See issue #2245 for more details.
Copy link
Member

Choose a reason for hiding this comment

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

@bryce-anderson This test is very important. It helps to validate that we correctly compute the effective strategy based on all default filters and modules bundled together by default. If we disable it completely, we open a risk to missing a bug that will have a significant performance hit (for example, if we add a filter with non-overridden requireOffloads() method). We had that in the past.

Instead of disabling all of them, consider using assumeThat(...) statement to disable (with comment) only those test cases that are flaky based on #2245 records. That will help us to make sure the effective strategy is computed correctly if at least some APIs are validated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants