Fix tests in Dev Container #965
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this PR, tests in classes deriving from
KestrelInMemoryTestwould fail using the Dev Container configured by this project's devcontainer.json based onmcr.microsoft.com/devcontainers/dotnet:1-8.0-jammy. TheHttpClientwould timeout trying to connect to Kestrel using the in-memory test transport. Some investigation uncovered this was caused by the Dockerfile setting a non-defaultASPNETCORE_HTTP_PORTSenvironment variable of8080instead of the normal default port of 5000 used by default by the test.I noticed that the tests using
SseServerIntegrationTestFixturestill passed on the Dev Container whileKestrelInMemoryTestwouldn't, and this is becauseModelContextProtocol.TestSseServerreferenced by the test fixture usesWebApplication.CreateEmptyBuilder()which doesn't read environment variables as configuration instead ofWebApplication.CreateSlimBuilder()which does. I could have fixed the default port issue by explicitly calling something likeListenLocalhost(5000)which would override configuration, but I discovered thatCreateEmptyBuilder()actually will automatically add the routing middleware if you don't add it manually just like the other builders despite what I claimed in the comment I deleted. Had I known that to begin with, I would have usedCreateEmptyBuilder()all along for simplicity and avoiding potential environmental issues like this.I also updated the Dev Container's "postCreateCommand" to install and trust a developer certificate. Technically, only the installation is necessary, since the tests use
RemoteCertificateValidationCallback = (_, _, _, _) => true, but I think trusting makes sense for running samples. Currently, only theTestOAuthServeruses HTTPS. This wouldn't be necessary if we updated the tests andProtectedMCPServersample to set RequireHttpsMetadata to false. But for the sample in particular, I'd rather not do this.