Skip to content

Port more tests to run on Linux#83052

Open
jaredpar wants to merge 42 commits intomainfrom
dev/jaredpar/port-o
Open

Port more tests to run on Linux#83052
jaredpar wants to merge 42 commits intomainfrom
dev/jaredpar/port-o

Conversation

@jaredpar
Copy link
Copy Markdown
Member

@jaredpar jaredpar commented Apr 3, 2026

This ports all our remaining unit test projects that target .NET Core to now run on Linux. The one exception is Microsoft.CodeAnalysis.CSharp.Features.UnitTests.csproj which requires a bit deeper analysis. This was done by running a ralph loop on the tests and porting. Each test class port is captured in a different commit. The commit will have details on the test ported, the failures encountered.

The actions from the agent can be seen here

Microsoft Reviewers: Open in CodeFlow

jaredpar and others added 30 commits April 2, 2026 15:16
…inux

74 multi-line raw string tests failed due to \r\n vs \n line endings in
source file. Added NormalizeLineEndings() call in the Test() helper to
ensure consistent \r\n input across platforms. All 133 tests now pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
All 59 SyntaxGeneratorTests failures were caused by line ending mismatch.
The VerifySyntax helper compares expected text (string literals with \n on
Linux) against NormalizeWhitespace() output (always \r\n). Fixed by adding
.NormalizeLineEndings() to the expectedText parameter in VerifySyntax.

Changed project target framework from $(NetRoslynWindowsTests) to
$(NetRoslyn) as this was the last todo item for this project.

All 193 tests now pass on Linux (net10.0).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Added platform guard in CreateWorkspace to downgrade TestHost.OutOfProcess
to TestHost.InProcess on non-Windows. All 200 tests were failing due to
TemporaryStorageService cast exception in OOP pipeline on Linux. The fix
follows the same guard pattern used in other test infrastructure.

All 232 tests now pass on Linux (net10.0).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Added platform guards in SolutionTestHelpers.CreateWorkspace and
  CreateWorkspaceWithPartialSemantics to downgrade TestHost.OutOfProcess
  to TestHost.InProcess on non-Windows (fixes 47 OOP TaskCanceledException
  failures caused by TemporaryStorageService cast issue)
- Changed UpdatingAnalyzerReferenceReloadsGenerators from informational
  [SupportedOSPlatform("windows")] to [ConditionalTheory(typeof(WindowsOnly))]
  since it explicitly requires OOP which is not supported on Linux

Result: 127 passed, 2 skipped, 0 failed

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
14 tests failed due to line ending mismatch. The VerifyAsync helper
normalized input and expected strings to \r\n via FixLineEndings but
did not normalize the actual CodeCleaner output. On Linux the VB
formatter produces \n for text outside the cleaned span. Added
FixLineEndings() around the actual result in the Assert.Equal call.

Result: 22 passed, 1 skipped (pre-existing), 0 failed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaced hardcoded Windows paths (C:\ProjectDirectory) with
TestPathUtil.GetRootedPath() for cross-platform absolute paths.
Replaced backslash-separated relative paths with Path.Combine()
for cross-platform directory separators. Converted rooted Windows
paths in command line args to use TestPathUtil and Path.Combine.
All 15 tests now pass on Linux.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…inux

Replaced hardcoded Windows paths (Z:\, C:\) with TestPathUtil.GetRootedPath() in 9
editorconfig and file access tests. Marked TestGetRecoveredTextAsync as
Windows-only due to GC timing differences on Linux (weak reference not
reliably released).

Result: 261 passed, 10 skipped (1 new Windows-only + 9 pre-existing), 0 failed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaced hardcoded C:\ Windows paths with TestPathUtil.GetRootedPath() for
cross-platform rooted paths. Updated expected results to use Path.Combine()
for cross-platform path separators. Marked GetRelativePath_OnADifferentDrive
as WindowsOnly since drive letters are a Windows-only concept.

7 tests fixed, 1 test marked Windows-only.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Both tests (TestFileNamesNotGenerated, TestFileNamesGenerated) failed with
ArgumentException due to hardcoded z:\ Windows paths for editorconfig and
document file paths. Replaced with TestPathUtil.GetRootedPath() for
cross-platform path handling.

Result: 2 passed, 0 skipped, 0 failed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Moved 2 failing InlineData rows using sources/**/* glob patterns with
Windows C:\ paths to a separate WindowsOnly test method. The ** glob
matching in SupportsFilePath does not handle Windows backslash separators
on Linux. Unix path equivalents already existed and pass.

99 passed, 1 skipped (Windows-only glob ** test), 0 failed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TestLoadProjectFromCommandLine failed due to hardcoded Windows path
C:\ProjectDirectory. Replaced with TestPathUtil.GetRootedPath and
Path.Combine for cross-platform path handling.

Result: 2 passed, 0 skipped, 0 failed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TestParseRoslynEditorConfig failed because hardcoded TextSpan positions
were based on \r\n line endings in the RoslynEditorConfigText const string.
On Linux the source has \n endings, shifting all character positions.

Fixed by calling .NormalizeLineEndings() on RoslynEditorConfigText before
passing to SourceText.From() to ensure consistent \r\n line endings.

Result: 70 passed, 0 skipped, 0 failed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Linux

Fixed FormatAsync_ForeignLanguageWithFormattingSupport assertion that hardcoded
\r\n as the expected default NewLine. On Linux the default is \n, so replaced
with dynamically computed Environment.NewLine. Changed project TFM from
$(NetRoslynWindowsTests) to $(NetRoslyn) as this is the last test class for
this project.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…yTests to Linux

4 tests failed due to multi-line raw string literals having \n line endings on
Linux instead of \r\n on Windows. The tests hardcode source span positions that
assume \r\n, causing position mismatches on Linux.

Fixed by adding .NormalizeLineEndings() to the raw string literals passed to
TestWorkspace.CreateCSharp() in the 4 affected tests:
- ToClassifiedDefinitionItemAsync_Namespace_MetadataAndSource
- ToClassifiedDefinitionItemAsync_Parameter
- ToClassifiedDefinitionItemAsync_MethodTypeParameter
- ToClassifiedDefinitionItemAsync_LocalVariable

Result: 29 passed, 0 skipped, 0 failed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dDebugInfoReaderTests to Linux

- Fixed 2 PortablePdb tests (with and without SymReader) by adding
  .NormalizeLineEndings() to source string to ensure consistent \r\n
  line endings across platforms (IL byte offsets shifted on Linux due
  to \n vs \r\n).
- Moved native PDB test case (DebugInformationFormat.Pdb) to separate
  DebugInfo_NativePdb method marked WindowsOnly since native PDB writing
  requires the Windows SymWriter COM component.
- Extracted shared logic into DebugInfoImpl private method.
- Result: 3 passed, 1 skipped (Windows-only), 0 failed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…eServiceTests to Linux

Proxy(OutOfProcess) failed with TemporaryStorageService cast exception in OOP
remote host infrastructure on Linux. Added platform guard to downgrade
TestHost.OutOfProcess to TestHost.InProcess on non-Windows, following the
same pattern used in FindAllDeclarationsTests and SolutionWithSourceGeneratorTests.

Result: 2 passed, 0 skipped, 0 failed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GetHotReloadDiagnostics failed on Linux because the mapped file path
@"..\a.razor" uses a Windows backslash separator. On Linux, backslash is
not a directory separator, so Path.GetFullPath cannot resolve the ".."
in the combined path. Changed to Path.Combine("..", "a.razor") for
cross-platform compatibility.

18 passed, 0 skipped, 0 failed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ests to Linux

Ordering test failed due to line ending mismatch: raw string literal source
has \n on Linux vs \r\n on Windows, shifting TextSpan byte offsets.
Fixed by adding .NormalizeLineEndings() to the source string.

Changed project target framework from $(NetRoslynWindowsTests) to $(NetRoslyn)
as this was the last todo item for this project.

16 passed, 0 skipped, 0 failed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Linux

- Fixed RangeToTextSpan tests: Added .NormalizeLineEndings() to raw string
  literals in GetTestMarkup() and RangeToTextSpanLineEndOfDocumentWithEndOfLineChars
  to ensure consistent \r\n line endings across platforms.
- Fixed CreateAbsoluteUri_LocalPaths_AllAscii: Corrected Unix uriPrefix from
  empty string to "_". Added skip for '?' on Unix due to inconsistent URI
  encoding between GetAbsoluteUriString and CreateAbsoluteDocumentUri.
- Fixed CreateAbsoluteUri_LocalPaths_Normalized_Unix: Changed LocalPath assertion
  to use Path.GetFullPath(filePath) matching the Windows test pattern.
- Fixed CreateAbsoluteUri_LocalPaths_Unix: Updated backslash expectations to use
  %5C encoding (Unix treats \ as literal). Removed non-absolute path test rows.
- Fixed CreateRelativePatternBaseUri_LocalPaths_Unix: Same backslash fixes.
  Removed root '/' test row (production code limitation). Removed non-absolute paths.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ts to Linux

Replaced hardcoded Windows paths (C:\...) with cross-platform paths
using Path.Combine(TempRoot.Root, filename) in all 5 test methods.
On Linux, C:\ paths are not rooted, so the workspace parser prepends
TempRoot.Root, causing assertion mismatches. Using TempRoot.Root
directly ensures paths work on both Windows and Linux.

All 18 tests pass on Linux (net10.0).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…inux

Replaced hardcoded \r\n line endings in expected TextEdit NewText values
with Environment.NewLine for cross-platform compatibility. Made cursor
position offset assertions platform-aware (14/13 and 268/259) to account
for \r\n vs \n line ending size differences.

10 tests fixed, 36 passed, 1 pre-existing skip.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixed 8 test failures caused by line ending mismatches on Linux:
- Replaced hardcoded \r\n with \n in inserted text strings for
  DidChange_MultipleChanges_ForwardOrder, DidChange_MultipleChanges_ReverseOrder,
  and DidChange_MultipleRequests
- Added NormalizeLineEndings() to expected string in LinkedDocuments_AllTextChanged
  since XML workspace parser produces \r\n documents regardless of platform

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- TestSemanticSnippetChangeAsync: Added NormalizeLineEndings() to assertion
  with hardcoded \r\n that didn't match \n output on Linux
- TestResolveOverridesCompletionItem_SnippetsEnabled_CaretOutOfSnippetScopeAsync:
  Changed hardcoded TextSpan(start: 77) to compute dynamically from document
  text, as offset was wrong on Linux due to LF vs CRLF line endings
- TestResolveCompletionItemWithPlainTextAsync: Added NormalizeLineEndings()
  to both expected and actual values for documentation string comparison

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Marked TestMiscDocument_WithFileScheme as WindowsOnly: uses C:\ paths and
  private-use Unicode character encoding differs across platforms
- Fixed TestWorkspaceDocument_WithFileScheme: replaced hardcoded C:\A.cs and
  C:\CSProj1.csproj with TestPathUtil.GetRootedPath for cross-platform paths

29 passed, 1 skipped (WindowsOnly), 0 failed

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Linux

Regex comment token length was hardcoded to 9 (includes \r from Windows CRLF).
On Linux with LF line endings, the token is 8 chars. Replaced with computed
value: 7 + Environment.NewLine.Length.

All 38 tests pass on Linux.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixed 4 test failures (2 tests × 2 CombinatorialData variants):
- TestGetHoverAsync_WithoutMarkdownClientSupport
- TestGetHoverAsync_UsingMarkupContentDoesNotEscapeCodeBlock

Root cause: Hover API output contains mixed \r\n line endings from XML doc
comment processing, while C# verbatim string literals use OS-native \n on
Linux.

Fix: Applied .ReplaceLineEndings() to both expected and actual values in
Assert.Equal calls to normalize line endings before comparison.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaced hardcoded Windows paths (C:\C.cs, C:\T.ts) with
TestPathUtil.GetRootedPath() in TestNoWorkspaceDiagnosticsForClosedFilesInProjectsWithIncorrectLanguage
for cross-platform compatibility. Updated assertion to compare against
the variable path instead of a hardcoded Windows string.

2 tests fixed, 30/30 passing on net10.0.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaced hardcoded Windows paths (C:\) with TestPathUtil.GetRootedPath() in:
- TestSpanMapper.s_mappedFilePath in AbstractLanguageServerProtocolTests.cs
- AddMappedDocument filePath in AbstractLanguageServerProtocolTests.cs
- DocumentUri in TestRename_WithMappedFileAsync
- Linked file workspace XML in TestRename_WithLinkedFilesAsync and
  TestRename_WithLinkedFilesAndPreprocessorAsync

2 initial failures (TestRename_WithMappedFileAsync x2), all 38 tests now pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Linux

Fixed TestFindImplementationAsync_MappedFile: replaced hardcoded C:\ path
with TestPathUtil.GetRootedPath for cross-platform compatibility.

All 12 tests passed on Linux (net10.0).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jaredpar and others added 7 commits April 3, 2026 03:51
…o Linux

Normalized raw-string markup and expected text with NormalizeLineEndings()
in VerifyCSharpMarkupAndExpectedRawString to fix 2 failing tests:
- OnAutoInsert_RawString_GrowDelimitersWhenEndExists_Interpolated (both
  mutatingLspWorkspace variants) failed because \n line endings on Linux
  caused the auto-insert handler to return null for the interpolated raw
  string case.

46 tests passed, 0 skipped, 0 failed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
OnAutoInsert_EnterKey3 marked as WindowsOnly: the bare /// comment
auto-insert handler returns null on Linux due to line ending
differences in the parser. All other 84 tests pass on Linux.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TestSnippetWithNoEditableFields failed (2 cases) due to line ending mismatch:
snippet processor outputs \r\n but Linux raw string literals use \n.

Fixed by normalizing line endings with ReplaceLineEndings("\r\n") on both
expected and actual values in VerifyMarkupAndExpected and TestSnippetCached.

Result: 22 passed, 0 skipped, 0 failed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaced hardcoded C:\ path in TestGotoDefinitionAsync_MappedFile with
TestPathUtil.GetRootedPath for cross-platform compatibility. The test was
constructing a DocumentUri using $"C:\\{TestSpanMapper.GeneratedFileName}"
which doesn't resolve on Linux.

All 34 tests in GoToDefinitionTests pass on net10.0/Linux.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Changed TFM from $(NetRoslynWindowsTests) to $(NetRoslyn) (last class for this project)
- Replaced hardcoded C:\C.cs path with TestPathUtil.GetRootedPath("C.cs") in TestLinkedDocuments
- Added .ReplaceLineEndings("\r\n") to both sides of Assert.Equal in TestLinkedDocuments
  to handle mixed line endings across platforms
- All 12 tests pass on Linux

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Skip WPF and WinForms templates (wpf*, winforms*) in GetProjectTemplateNames()
on non-Windows platforms. These templates target net10.0-windows and fail with
NETSDK1100 on Linux. Follows same pattern as existing Maui template exclusion.

14 failures fixed, 22 tests pass on Linux.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Changed TFM from $(NetRoslynWindowsTests) to $(NetRoslyn) (last item for project)
- Fixed path normalization in WorkspaceTestBase: added NormalizePath() helper
  to replace backslashes with forward slashes on Unix, applied in
  GetSolutionFileName() and CreateFiles()
- Fixed DotNetRestore/DotNetBuild in NetCoreTests to normalize paths passed
  to dotnet CLI
- All 17 tests pass on Linux

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
public async Task Proxy(TestHost testHost)
{
if (!ExecutionConditionUtil.IsWindows && testHost == TestHost.OutOfProcess)
testHost = TestHost.InProcess;
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 TestHost.OutOfProcess is definitely busted on Unix. It's the reason that I delayed porting Microsoft.CodeAnalysis.CSharp.Features.UnitTests.csproj (10,000+ failures). Not sure how you all want to track this but happy to file an issue, tag and come back.

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.

Filed this issue to track. Already have a fix locally and will be in the next PR of this series.

#83054

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.

Tracking bug is fine for me. Just something we can grep for and clean up later.


var expectedEdit = new TextEdit { Range = new LSP.Range { Start = new(7, 4), End = new(7, 13) }, NewText = "public override global::System.Boolean AbstractMethod(global::System.Int32 x)\r\n {\r\n throw new System.NotImplementedException();\r\n }" };
var nl = Environment.NewLine;
var expectedEdit = new TextEdit { Range = new LSP.Range { Start = new(7, 4), End = new(7, 13) }, NewText = $"public override global::System.Boolean AbstractMethod(global::System.Int32 x){nl} {{{nl} throw new System.NotImplementedException();{nl} }}" };
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.

It's interesting to see that in most cases the AI did "...{Environment.NewLine}" but in this case factored out to a local. guessing in this case it was triggering a line is too long behavior. Happy to change or keep as is.

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.

I'd say inline it, but asking you to rev the PR just for that seems silly.

Comment on lines -100 to +106
[InlineData("/\\\u200e//", "file:////%E2%80%8E//")] // cases from https://github.com/dotnet/runtime/issues/1487
[InlineData("\\/\u200e", "file:////%E2%80%8E")]
[InlineData("/\\\\-Ā\r", "file://///-%C4%80%0D")]
[InlineData("\\\\\\\\\\\u200e", "file:///////%E2%80%8E")]
[InlineData("/\\\u200e//", "file:///%5C%E2%80%8E//")] // cases from https://github.com/dotnet/runtime/issues/1487
[InlineData("/\\\\-Ā\r", "file:///%5C%5C-%C4%80%0D")]
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.

I'm not quite sure what to do with this. Yes Copilot deleted test cases here. But this is a test that is marked as "unix only" which is not actually running on Unix in CI. I can dig deeper on this.

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.

Can either dig into it, or at least file an issue to add these back and I can look at it later. Don't want to lose coverage here without understanding why.

@jaredpar jaredpar marked this pull request as ready for review April 3, 2026 23:36
@jaredpar jaredpar requested review from a team as code owners April 3, 2026 23:36
@jaredpar
Copy link
Copy Markdown
Member Author

jaredpar commented Apr 3, 2026

@jasonmalinowski, @akhera99, @JoeRobich PTAL.

333fred
333fred previously approved these changes Apr 4, 2026
Copy link
Copy Markdown
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Did some spot checks of files, haven't gone through every line. Not sure about some of these english only changes.

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.

Think we can either integrate this into TestHelpers.cs, or pull the path-related helpers from there into here.

}

[Theory, CombinatorialData]
[ConditionalTheory(typeof(IsEnglishLocal)), CombinatorialData]
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.

Odd that these need English specified. There's a Windows spanish queue that these, presumably, were already running in. What's different about Linux?

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 Linux test runs set both CurrentCulture and CurrentUICulture to es-es where Windows only sets the former. This is why you see new issues when we port more to Linux. Documentation.

Once we're done with all the Linux port I think we should consider removing the Windows Spanish leg. It's a constant source of problems for us as roslyn is the only one using the queue. We are going to get enough coverage on Linux that we can likely feel good about our implementation.

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.

I'm still confused. That doc states that we override CurrentUICulture ourselves, so what actually causes the issue?

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.

Yes but this is the key part of the doc

This is done in the TestBase constructor, which all compiler test classes inherit from

This means only the majority of compiler tests have this behavior. Tests in other layers, like this one, don't inherit from this type hence they get the machine defaults. This is yet another reason why I like our Linux setup better, it's more complete.

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.

That's what I was missing, thanks

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.

Can we summarize this conversation into a comment into the source here, or add a TODO to track fixing this? I imagine xunit might have some useful alternatives here we might want to investigate.

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.

A little hesitant about this. I can't remember for certain, but I have a decent recollection that the windows spanish leg has caught issues with these tests. Think this definitely deserves a tracking issue (can assign to me) to get this coverage back (fine with the linux leg for that).

Once we're done with all the Linux port I think we should consider removing the Windows Spanish leg

We won't have any coverage on the VS code for spanish then?

@333fred 333fred dismissed their stale review April 4, 2026 00:47

Meant to click comment, not approve.

{
var textChange = new TextChange(span: new TextSpan(start: 77, length: 9), newText: """
var text = await document.GetTextAsync(cancellationToken);
var start = text.ToString().IndexOf("override ");
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.

not clear why it changed how it got the start value?

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.

I'm guessing because there's newlines before it and that will change the exact position?

{WorkspacesResources.Exceptions_colon}
System.NullReferenceException
", results.Contents.Fourth.Value);
".ReplaceLineEndings(), results.Contents.Fourth.Value.ReplaceLineEndings());
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.

unclear to me where it chose to ReplaceLineEndings vs. NormalizeLineEndings?

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.

I imagine we'll want to do a pass to get our tests down to a single helper here. Or actually (counterintuitively) remove these helpers and instead ensure our product is actually handing the other line endings properly in the first place.

I had asked @jaredpar to at least get everything calling these, since we can audit them further. There's already a lot of abuse of these methods as it is. Why we even have so many helpers is confusing to me. 😄

Copy link
Copy Markdown
Member

@dibarbet dibarbet Apr 6, 2026

Choose a reason for hiding this comment

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

Or actually (counterintuitively) remove these helpers and instead ensure our product is actually handing the other line endings properly in the first place.

I agree. I haven't done any digging but any usage of these here seems like a bug where the test or product code isn't doing something right. Fine witha followup assigned to me.

Comment on lines +52 to +53
[ConditionalFact(typeof(WindowsOnly), Reason = "Native PDB writing requires Windows")]
public void DebugInfo_NativePdb()
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.

Not something you can do now, but xunit 3 brings the ability to put an Assert.Skip which would be handy for cases like this. If you see more or have AI instructions to tweak, you could at least mention putting a TODO here to later clean up.

public async Task Proxy(TestHost testHost)
{
if (!ExecutionConditionUtil.IsWindows && testHost == TestHost.OutOfProcess)
testHost = TestHost.InProcess;
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.

Tracking bug is fine for me. Just something we can grep for and clean up later.

}

[Theory, CombinatorialData]
[ConditionalTheory(typeof(IsEnglishLocal)), CombinatorialData]
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.

Can we summarize this conversation into a comment into the source here, or add a TODO to track fixing this? I imagine xunit might have some useful alternatives here we might want to investigate.


var expectedEdit = new TextEdit { Range = new LSP.Range { Start = new(7, 4), End = new(7, 13) }, NewText = "public override global::System.Boolean AbstractMethod(global::System.Int32 x)\r\n {\r\n throw new System.NotImplementedException();\r\n }" };
var nl = Environment.NewLine;
var expectedEdit = new TextEdit { Range = new LSP.Range { Start = new(7, 4), End = new(7, 13) }, NewText = $"public override global::System.Boolean AbstractMethod(global::System.Int32 x){nl} {{{nl} throw new System.NotImplementedException();{nl} }}" };
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.

I'd say inline it, but asking you to rev the PR just for that seems silly.

{
var textChange = new TextChange(span: new TextSpan(start: 77, length: 9), newText: """
var text = await document.GetTextAsync(cancellationToken);
var start = text.ToString().IndexOf("override ");
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.

I'm guessing because there's newlines before it and that will change the exact position?

protected override TestComposition Composition => base.Composition.AddParts(typeof(CustomResolveHandler));

[Theory, CombinatorialData]
[ConditionalTheory(typeof(WindowsOnly), Reason = "Uses Windows paths and Unicode encoding differs across platforms")]
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.

Not clear to me why this couldn't have been fixed. Or if this indicates a temporary state or something that needs a tracking bug.

{
var commandLine = @"goo.cs subdir\bar.cs /out:goo.dll /target:library";
var info = CommandLineProject.CreateProjectInfo("TestProject", LanguageNames.CSharp, commandLine, @"C:\ProjectDirectory");
var baseDir = TestPathUtil.GetRootedPath("ProjectDirectory");
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.

Nit: put baseDir after commandLine; otherwise I had to check that we didn't "helpfully" use baseDir in the next expression which defeats part of the point of the test.

Assert.True(section.SupportsFilePath(codefilePath, matchKind: SectionMatch.SplatMatch));
}

[ConditionalTheory(typeof(WindowsOnly), Reason = "Glob ** with Windows paths requires Windows path handling")]
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.

Should we have an equivalent test for the Unix side then?


[MethodImpl(MethodImplOptions.NoInlining)]
[Fact]
[ConditionalFact(typeof(WindowsOnly), Reason = "GC timing differs on Linux, reference not reliably released")]
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.

Please file a bug for this.

}

// WPF and WinForms templates require Windows targeting and fail with
// NETSDK1100 on non-Windows platforms.
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.

I'm not quite sure with what setting the EnableWindowsTargeting might mean, but consider filing a follow up of whether we want to update the test to set it.

Comment on lines -100 to +106
[InlineData("/\\\u200e//", "file:////%E2%80%8E//")] // cases from https://github.com/dotnet/runtime/issues/1487
[InlineData("\\/\u200e", "file:////%E2%80%8E")]
[InlineData("/\\\\-Ā\r", "file://///-%C4%80%0D")]
[InlineData("\\\\\\\\\\\u200e", "file:///////%E2%80%8E")]
[InlineData("/\\\u200e//", "file:///%5C%E2%80%8E//")] // cases from https://github.com/dotnet/runtime/issues/1487
[InlineData("/\\\\-Ā\r", "file:///%5C%5C-%C4%80%0D")]
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.

Can either dig into it, or at least file an issue to add these back and I can look at it later. Don't want to lose coverage here without understanding why.

}

[Theory, CombinatorialData]
[ConditionalTheory(typeof(IsEnglishLocal)), CombinatorialData]
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.

A little hesitant about this. I can't remember for certain, but I have a decent recollection that the windows spanish leg has caught issues with these tests. Think this definitely deserves a tracking issue (can assign to me) to get this coverage back (fine with the linux leg for that).

Once we're done with all the Linux port I think we should consider removing the Windows Spanish leg

We won't have any coverage on the VS code for spanish then?

private static void Test(string stringText, string expected, ParseOptions? options = null)
{
// Normalize line endings so multi-line raw string expectations (authored with \r\n) work on all platforms.
stringText = stringText.NormalizeLineEndings();
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.

Probably a dumb question - I was under the understanding that raw strings would use the line endings from the source. But if we're building on unix, wouldn't the source be checked out with \n, and hence should match already?

{WorkspacesResources.Exceptions_colon}
System.NullReferenceException
", results.Contents.Fourth.Value);
".ReplaceLineEndings(), results.Contents.Fourth.Value.ReplaceLineEndings());
Copy link
Copy Markdown
Member

@dibarbet dibarbet Apr 6, 2026

Choose a reason for hiding this comment

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

Or actually (counterintuitively) remove these helpers and instead ensure our product is actually handing the other line endings properly in the first place.

I agree. I haven't done any digging but any usage of these here seems like a bug where the test or product code isn't doing something right. Fine witha followup assigned to me.

Copy link
Copy Markdown
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Approving, but generally would appreciate more comments on the ones being conditioned on the English locale to distinguish between "this test is asserting English strings, and it's non-trivial to fix that" versus more the "this is how we set up our tests and really should be fixed".

<RootNamespace>Microsoft.CodeAnalysis.UnitTests</RootNamespace>
<!-- Test our primary scenarios: VS OOP, VS Code, devenv -->
<TargetFrameworks>$(NetRoslynWindowsTests);net472</TargetFrameworks>
<TargetFrameworks>$(NetRoslyn);net472</TargetFrameworks>
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.

Was originally NetVSShared.

Suggested change
<TargetFrameworks>$(NetRoslyn);net472</TargetFrameworks>
<TargetFrameworks>$(NetVSShared);net472</TargetFrameworks>

<PropertyGroup>
<StartupObject />
<TargetFrameworks>$(NetRoslynWindowsTests);net472</TargetFrameworks>
<TargetFrameworks>$(NetRoslyn);net472</TargetFrameworks>
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.

Was originally NetVSCode

Suggested change
<TargetFrameworks>$(NetRoslyn);net472</TargetFrameworks>
<TargetFrameworks>$(NetVSCode);net472</TargetFrameworks>

<RootNamespace>Microsoft.CodeAnalysis.CSharp.UnitTests</RootNamespace>
<!-- Test our primary scenarios: VS OOP, VS Code, devenv -->
<TargetFrameworks>$(NetRoslynWindowsTests);net472</TargetFrameworks>
<TargetFrameworks>$(NetRoslyn);net472</TargetFrameworks>
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.

Suggested change
<TargetFrameworks>$(NetRoslyn);net472</TargetFrameworks>
<TargetFrameworks>$(NetVSShared);net472</TargetFrameworks>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants