Conversation
Change TFM from NetRoslynWindowsTests to NetRoslyn. Mark 15 tests as WindowsOnly: - 7 deep recursion tests that rely on Windows ~1MB stack size - 8 JSON parser tests with line-ending-dependent diagnostic offsets Passed: 27531, Failed: 0, Skipped: 63 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@jasonmalinowski, @333fred, @akhera99, @dotnet/roslyn-infrastructure PTAL |
| """); | ||
|
|
||
| [Fact] | ||
| [ConditionalFact(typeof(WindowsOnly))] // Diagnostic offsets differ due to \r\n vs \n line endings |
There was a problem hiding this comment.
This is the analysis that Copilot had for the test failure, which is accurate. How do you all want this actually written up in code? Could have it put the comment as the skip reason.
There was a problem hiding this comment.
I'd say put them in the skip reason, since that way it'll be easier to track long term that we've fixed up tests like this.
There was a problem hiding this comment.
Alternatively, I think line ending issues are going to be a really common problem breaking tests; consider maybe making a ConditionalFact(typeof(WindowsOnlyBecauseOfLineEndings)) which at some later point we can easily tell Copilot to get rid of and try fixing all of them up. Or maybe a WindowsOnlyButFixable.
There was a problem hiding this comment.
Joey and I have a PR out where we try to fix up those line endings issues (or told Copilot to try more aptly) #82527
These tests compare regex diagnostic messages which are localized by .NET on non-English locale machines (e.g. Spanish CI leg). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jasonmalinowski
left a comment
There was a problem hiding this comment.
Signing off but to summarize the feedback:
It might be good for us to put the issues in a skip reason, or make a WindowsOnlyButFixable conditional fact, or something to make the fact these are being skipped not for fundamental reasons but simply because there's some incidental bug that's making them not work. I don't think I have a strong preference, but consider the case where we (later) want to tell Copilot to go and fix all these up. Can we do this in some way that means we can tell Copilot "all the tests marked with " can be fixed -- can you go and do so? It's a bit murky how to give that instruction if there's just a comment.
| => $"(?({regex})[0-[0]]|.*)"; | ||
|
|
||
| [Fact] | ||
| [ConditionalFact(typeof(WindowsOnly))] // Deep recursion test relies on Windows stack size (~1MB) to trigger stack overflow; Linux has 8MB stack |
There was a problem hiding this comment.
If we just make the string bigger, can we hit it easily enough?
| """, RegexOptions.None); | ||
|
|
||
| [Fact] | ||
| [ConditionalFact(typeof(IsEnglishLocal))] |
There was a problem hiding this comment.
I don't expect this to be locale sensitive so we should have a tracking bug on this one.
| <Diagnostics> | ||
| <Diagnostic Message="Unrecognized grouping construct" Span="[12..13)" Text="(" /> | ||
| <Diagnostic Message="Quantifier '?' following nothing" Span="[13..14)" Text="?" /> | ||
| </Diagnostics> |
There was a problem hiding this comment.
@jasonmalinowski this message is a diagnostic from the underlying Regex. This is the raw message which is localized on Spanish runs hence fails here where it's hard coded to English. That is what I'm seeing at least.
There was a problem hiding this comment.
@jaredpar: ah, I think we have a different Fact attribute for forcing stuff to the English locale? That might be a better fix than skipping the test in that case. (That'd be a good copilot follow up.)
Change TFM from NetRoslynWindowsTests to NetRoslyn. Mark 15 tests as WindowsOnly:
Passed: 27531, Failed: 0, Skipped: 63
Microsoft Reviewers: Open in CodeFlow