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.

Sign up for GitHub

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

Jump to bottom

Mark tests flaky instead of skipped #8727

Merged
merged 5 commits into from Mar 26, 2019
Merged

Mark tests flaky instead of skipped #8727

merged 5 commits into from Mar 26, 2019

Conversation

ryanbrandenburg
Copy link
Contributor

This should make sure there's plenty to hear about once we finish https://github.com/aspnet/AspNetCore-Internal/issues/1948.

BrennanConroy
BrennanConroy reviewed Mar 22, 2019
View reviewed changes
src/SignalR/server/StackExchangeRedis/test/RedisHubLifetimeManagerTests.cs Outdated
@@ -37,7 +37,8 @@ private RedisHubLifetimeManager<MyHub> CreateLifetimeManager(TestRedisServer ser
}, NullLogger<DefaultHubProtocolResolver>.Instance));
}

[Fact(Skip = "https://github.com/aspnet/SignalR/issues/3088")]
[Fact]
[Flaky("https://github.com/aspnet/SignalR/issues/3088")]
Copy link
Member

Choose a reason for hiding this comment

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

This one isn't flaky, it's just broken. Do we want to keep it as skip or no?

jkotalik
jkotalik reviewed Mar 22, 2019
View reviewed changes
src/Servers/IIS/IIS/test/Common.FunctionalTests/AppOfflineTests.cs Outdated
@@ -77,7 +77,8 @@ public async Task AppOfflineDroppedWhileSiteFailedToStartInShim_AppOfflineServed
DeletePublishOutput(deploymentResult);
}

[ConditionalFact(Skip = "https://github.com/aspnet/IISIntegration/issues/933")]
[ConditionalFact]
[Flaky("https://github.com/aspnet/IISIntegration/issues/933")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this test. There has been no action on fixing https://github.com/dotnet/core-setup/issues/4646 and it isn't our problem.

jkotalik
jkotalik suggested changes Mar 22, 2019
View reviewed changes
src/Servers/IIS/IIS/test/Common.FunctionalTests/OutOfProcess/AspNetCorePortTests.cs Outdated
@@ -37,7 +37,8 @@ public static IEnumerable<object[]> InvalidTestVariants
from s in new string[] { (_minPort - 1).ToString(), (_maxPort + 1).ToString(), "noninteger" }
select new object[] { v, s };

[ConditionalTheory(Skip = "https://github.com/aspnet/AspNetCore/issues/8329")]
[ConditionalTheory]
[Flaky("https://github.com/aspnet/AspNetCore/issues/8329")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't enable these tests. We need an SDK upgrade to enable these

Copy link
Contributor

Choose a reason for hiding this comment

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

#8329

src/Servers/IIS/IIS/test/Common.FunctionalTests/OutOfProcess/HelloWorldTest.cs Outdated
@@ -26,7 +26,8 @@ public static TestMatrix TestVariants
.WithTfms(Tfm.NetCoreApp30)
.WithAllApplicationTypes();

[ConditionalTheory(Skip = "https://github.com/aspnet/AspNetCore/issues/8329")]
Copy link
Contributor

Choose a reason for hiding this comment

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

These too.

src/Servers/IIS/IIS/test/IIS.Tests/ClientDisconnectTests.cs Outdated
@@ -173,7 +173,7 @@ public async Task WriterThrowsCancelledException()
}
}

[ConditionalFact]
[Fact]
Copy link
Contributor

Choose a reason for hiding this comment

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

These need to be ConditionalFacts still. There are conditions applied on the entire assembly to check whether we are running on a machine that supports this test.

src/Servers/IIS/IIS/test/IISExpress.FunctionalTests/OutOfProcess/NtlmAuthentationTest.cs Outdated
@@ -29,7 +29,8 @@ public static TestMatrix TestVariants
=> TestMatrix.ForServers(DeployerSelector.ServerType)
.WithTfms(Tfm.NetCoreApp30);

[ConditionalTheory(Skip = "https://github.com/aspnet/AspNetCore/issues/8329")]
[ConditionalTheory]
[Flaky("https://github.com/aspnet/AspNetCore/issues/8329")]
Copy link
Contributor

Choose a reason for hiding this comment

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

These three. At this point I think you can just undo all changes to the src/Servers/IIS subfolder.

@Tratcher
Copy link
Member

How many of these fail every time? Not much point in marking them as flaky.

@analogrelay
Copy link
Contributor

I think we should review all the ones we know about and keep the “consistently broken” ones as Skipped (or delete them). There are bound to be ones we don’t know/remember well though. Might be worth doing a survey of the flaky tests in, say, a week and for any of them that have never passed in that time, switch them back to fully skipped. Thoughts?

Sent with GitHawk

@ryanbrandenburg
Copy link
Contributor Author

I was going to go with "unskip everything and let it make noise" but I'm fine with leaving consistently failing ones out.

@ryanbrandenburg
Copy link
Contributor Author

🆙📅 I reviewed the issues and left the ones that claim to fail every time as skipped.

BrennanConroy
BrennanConroy approved these changes Mar 22, 2019
View reviewed changes
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Good from SignalR and dotnet-watch side

@Eilon Eilon added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-servers area-signalr Includes: SignalR clients and servers labels Mar 22, 2019
@ryanbrandenburg ryanbrandenburg merged commit 70cd5f5 into master Mar 26, 2019
@ryanbrandenburg ryanbrandenburg deleted the rybrande/SkipToFlaky branch March 26, 2019 16:51
@amcasey amcasey added area-hosting Includes Hosting area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tratcher Tratcher Tratcher approved these changes

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@jkotalik jkotalik jkotalik approved these changes

@analogrelay analogrelay Awaiting requested review from analogrelay

@halter73 halter73 Awaiting requested review from halter73

@mikaelm12 mikaelm12 Awaiting requested review from mikaelm12

@SteveSandersonMS SteveSandersonMS Awaiting requested review from SteveSandersonMS

Assignees
No one assigned
Labels
area-hosting Includes Hosting area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions area-signalr Includes: SignalR clients and servers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ryanbrandenburg @Tratcher @analogrelay @BrennanConroy @jkotalik @Eilon @amcasey