Allow insecure requests on debug builds via OSU_INSECURE_REQUESTS environment variable
#6283
Conversation
The top of this file was a bit of a mess and not matching our 2024 code standards.
Odd case that Rider misses.
| host = "127.0.0.1:8080"; | ||
| protocols = new[] { default_protocol }; | ||
| } | ||
| else | ||
| { | ||
| host = "httpbin.org"; | ||
| protocols = new[] { default_protocol, "https" }; |
There was a problem hiding this comment.
This feels a bit weird no?
... like I'd have to check how the tests are being run as to whether this is getting full test coverage or not.
There was a problem hiding this comment.
This is copied from TestWebRequest, so I can't provide much other than this is how it's been done so far. Do you have any suggestions?
There was a problem hiding this comment.
I tried changing this to skip the "https" tests rather than not list them at all, but it's only marginally better. I don't have any better ideas so let's let this one pass.
| byte[]? result = store.Get(null); | ||
| Assert.That(result, Is.Null); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd hope for test coverage of file:// not working.
|
Goes without saying but hold off on merging this. The linux test is weird and suffers from |
With the way the test runner works, this attribute ends up calling into FrameworkEnvironment during test _discovery_. At this point, the "currently executing test" is an adhoc test internal to NUnit, leading `DebugUtils.NUnitTestAssembly` to discover the incorrect assembly.
smoogipoo
force-pushed
the
insecure-requests-via-env
branch
from
96deaf7
to
c035127
Compare
|
I did not mean to force push... |
In this test, the web request triggers an exception which should be correctly caught by the store methods.
|
c035127 should fix the Release-mode test on Linux, but consider it a bit of an RFC. The main purpose is to delay poking The commit message contains an explanation of why it was done. |
OSU_INSECURE_REQUESTS).WebRequest.AllowInsecureRequestsupdated to act as an override of the environment variable.OnlineStore.GetAsync(). This existed originally but was removed and incorrectly reverted at some point. Can't seem to pinpoint the exact commit where this happened.OnlineStoreto check for eitherhttporhttpschemes. Added tests.WIki entry: https://github.com/ppy/osu-framework/wiki/Environment-variables#osu_insecure_requests