-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add some HttpListener authentication tests #20102
Conversation
| public class Helpers | ||
| public static class Helpers | ||
| { | ||
| public static bool IsWindowsImplementation { get; } = false;//PlatformDetection.IsWindows && PlatformDetection.IsNotOneCoreUAP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this always false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's an accident when I was testing the managed implementation.
I basically discovered that the managed tests can be run on Windows which means I can debug them very easily using VS.
By using ConditionalFact/ConditionalTheory I can run managed tests on Windows without hanging tests and spammy failures.
The idea behind these is that the difference isn't between windows/unix so doesn't really fit into TestPlatforms, but rather managed and Windows, where the managed implementation can also run on windows
| await ValidateValidUser(); | ||
| } | ||
|
|
||
| [ConditionalTheory(nameof(Helpers) + "." + nameof(Helpers.IsWindowsImplementation))] // [ActiveIssue(20099, TestPlatforms.Unix)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this ConditionalTheory construct being used like this with IsWindowsImplementation? It doesn't work to just add the active issue for unix?
[ActiveIssue(20099, TestPlatforms.AnyUnix)]
[ConditionalTheory(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotOneCoreUAP))]?
| } | ||
|
|
||
| [ConditionalFact(nameof(Helpers) + "." + nameof(Helpers.IsWindowsImplementation))] // [ActiveIssue(20101, TestPlatforms.AnyUnix)] | ||
| [ActiveIssue(20096)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, why can't this be:
[ActiveIssue(20096, TestPlatforms.Windows)]
[ActiveIssue(20101, TestPlatforms.AnyUnix)]
[ConditionalFact(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotOneCoreUAP))] ? Does that not work?
I don't know why these are failing but I don't think it's a bug as Windows and managed share the same code here
| Assert.Throws<ObjectDisposedException>(() => listener.Realm = null); | ||
| } | ||
|
|
||
| public async Task<HttpResponseMessage> AuthenticationFailure(HttpClient client, HttpStatusCode errorCode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: we should make this private instead of public so that it's clear it's not a mistake that it doesn't have a Theory attribute on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(we don't have to restart CI for this though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
@dotnet-bot test outerloop netfx Windows_NT Debug |
|
@mmitche, do you know why these runs I triggered aren't getting executed? |
|
@stephentoub Azure stopped talking to us again. I'm rebooting it now. This should be fixed this week (made some tweaks to the plugin) |
* Add HttpListener authorization tests * Baseline managed test failures * Fix non-Windows test failures I don't know why these are failing but I don't think it's a bug as Windows and managed share the same code here
Lots more managed test failures, sorry - also a Windows implementation bug. This prevented me from adding more tests, but I verified that the disabled tests pass on netfx
Contributes to #13618