-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Delete default port tests #12983
Conversation
@anurse you had one alternate proposal to containerize these? |
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. |
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. |
Perhaps, but such infrastructure could be used for a lot more than just these tests. |
Here's a spicy question: Have these tests ever caught a regression that we wouldn't expect CTI to catch? |
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). |
Can we configure them to skip on the CI but to run locally? E.g. |
Alright between:
What do people prefer? |
I prefer just skipping on the CI, but I don't feel too strongly either way. |
Agreed, skipping on the CI gets rid of the flakyness and still lets us have some refactor coverage. |
I disagree but 2 against 1, I won't fight it. |
I'm going to make a change to extensions to do a few things: |
Waiting on dotnet/extensions#2186. |
56ec771
to
d42098c
Compare
Can someone hit me with approval for this? |
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.