Skip to content

Delete default port tests #12983

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

Merged
merged 1 commit into from
Aug 26, 2019
Merged

Delete default port tests #12983

merged 1 commit into from
Aug 26, 2019

Conversation

jkotalik
Copy link
Contributor

@jkotalik jkotalik commented Aug 8, 2019

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 halter73 and Tratcher August 8, 2019 18:01
@jkotalik jkotalik requested a review from analogrelay as a code owner August 8, 2019 18:01
@Tratcher
Copy link
Member

Tratcher commented Aug 8, 2019

@anurse you had one alternate proposal to containerize these?

@halter73
Copy link
Member

halter73 commented Aug 8, 2019

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
Copy link
Contributor Author

jkotalik commented Aug 8, 2019

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
Copy link
Member

Tratcher commented Aug 8, 2019

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

@analogrelay
Copy link
Contributor

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

@analogrelay
Copy link
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
Copy link
Member

Tratcher commented Aug 9, 2019

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
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
Member

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

@Tratcher
Copy link
Member

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

@jkotalik
Copy link
Contributor Author

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

@jkotalik
Copy link
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
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
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