Skip to content

Delete default port tests#12983

Merged
jkotalik merged 1 commit into
release/3.0from
jkotalik/httpsFlakyTest
Aug 26, 2019
Merged

Delete default port tests#12983
jkotalik merged 1 commit into
release/3.0from
jkotalik/httpsFlakyTest

Conversation

@jkotalik

@jkotalik jkotalik commented Aug 8, 2019

Copy link
Copy Markdown
Contributor

Fixes https://github.com/aspnet/AspNetCore-Internal/issues/2854

These tests are never reliable and fail frequently. We can't rely on binding to a static port, even with the extra double-check we do before running the test. I'd rather just delete them or make them manually ran tests. These scenarios are covered by CTI too.

@jkotalik jkotalik requested review from Tratcher and halter73 August 8, 2019 18:01
@jkotalik jkotalik requested a review from analogrelay as a code owner August 8, 2019 18:01
@Tratcher

Tratcher commented Aug 8, 2019

Copy link
Copy Markdown
Member

@anurse you had one alternate proposal to containerize these?

@halter73

halter73 commented Aug 8, 2019

Copy link
Copy Markdown
Member

My only question is if we should come up with something to replace these tests before removing them. Maybe CTI testing is enough, but part of the reason we haven't removed these yet is because doing so would likely decrease our motivation to add less/non-flaky replacement tests.

@jkotalik

jkotalik commented Aug 8, 2019

Copy link
Copy Markdown
Contributor Author

I think the cost-benefit of keeping/replacing this test is fairly high. Let's say we want to spin up containers to verify default port selection. Then we would need to set up infrastructure to run those either manually or automatically. My guess is it would take a week to get that work done. And the benefit is low: CTI covers default port selection well. We also still test IPV4 and IPV6 selection.

@Tratcher

Tratcher commented Aug 8, 2019

Copy link
Copy Markdown
Member

Perhaps, but such infrastructure could be used for a lot more than just these tests.

@analogrelay

Copy link
Copy Markdown
Contributor

Here's a spicy question: Have these tests ever caught a regression that we wouldn't expect CTI to catch?

@analogrelay

Copy link
Copy Markdown
Contributor

Containerizing is a solution but has a non-zero cost (from past experience, docker-related tests can themselves be flaky, plus they wouldn't work on macOS). I don't know that I see a big enough benefit to make it worth the cost of having these tests (assuming we have adequate CTI coverage).

@Tratcher

Tratcher commented Aug 9, 2019

Copy link
Copy Markdown
Member

Can we configure them to skip on the CI but to run locally? E.g. [SkipOnCI]. These tests are primarily useful during development to check for refactor regressions.

@jkotalik

Copy link
Copy Markdown
Contributor Author

Alright between:

  • Delete these tests
  • Keeping tests and always skipping them on CI (which requires a new attribute)

What do people prefer?

@analogrelay analogrelay added area-servers tell-mode Indicates a PR which is being merged during tell-mode labels Aug 12, 2019
@analogrelay analogrelay added this to the 3.0.0-preview9 milestone Aug 12, 2019
@halter73

Copy link
Copy Markdown
Member

I prefer just skipping on the CI, but I don't feel too strongly either way.

@Tratcher

Copy link
Copy Markdown
Member

Agreed, skipping on the CI gets rid of the flakyness and still lets us have some refactor coverage.

@jkotalik

Copy link
Copy Markdown
Contributor Author

I disagree but 2 against 1, I won't fight it.

@jkotalik

Copy link
Copy Markdown
Contributor Author

I'm going to make a change to extensions to do a few things:

  • Move the SkipOnHelix attribute to Extensions
  • Create a new attribute SkipOnCI
  • Remove a bunch of copied contents in all tests. Today, all test projects have like 10 extra files that only a few tests need:
    image
    This includes the SkipOnHelixAttribute.

@jkotalik

Copy link
Copy Markdown
Contributor Author

Waiting on dotnet/extensions#2186.

@jkotalik jkotalik force-pushed the jkotalik/httpsFlakyTest branch from 56ec771 to d42098c Compare August 19, 2019 18:01
@jkotalik

Copy link
Copy Markdown
Contributor Author

Can someone hit me with approval for this?

@jkotalik jkotalik merged commit 2d8cd17 into release/3.0 Aug 26, 2019
@ghost ghost deleted the jkotalik/httpsFlakyTest branch August 26, 2019 21:07
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions tell-mode Indicates a PR which is being merged during tell-mode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants