Skip to content

PR feedback from #11412#11474

Merged
davidfowl merged 1 commit into
masterfrom
davidfowl/https-fixes
Jun 24, 2019
Merged

PR feedback from #11412#11474
davidfowl merged 1 commit into
masterfrom
davidfowl/https-fixes

Conversation

@davidfowl

Copy link
Copy Markdown
Member
  • Remove ExpectedMiddlewareCount since everything is middleware now
  • Renamed everything adapter to middleware
  • Added a regression test for an https scenario
  • Don't send client certs for tests that don't expect it

PS: This was supposed to take 30 mins...

- Remove ExpectedMiddlewareCount since everything is middleware now
- Renamed everything adapter to middleware
- Added a regression test for an https scenario
- Don't send client certs for tests that don't expect it

options.ConfigureEndpointDefaults(e =>
{
if (e.IPEndPoint.Port == 5001)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you didn't have IsDevCertLoaded = true, above, this test would pass even without the custom ConfigureEndpointDefaults, right?

I think this test would be a little more convincing if it made port 5000 the TLS endpoint.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The default binding only checks 5001 for IsTls

@halter73

Copy link
Copy Markdown
Member

Why don't you rename HttpsConnectionAdapterTests to HttpsConnectionMiddlewareTests? :trollface:

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants