Run test on Linux and Mac

[Youssef1313]

This used to fail. I wanted to confirm if this is still an issue.

[ghost]

Thanks for your PR, @Youssef1313. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[Youssef1313]

@captainsafia I found an interesting thing here. The clone of the repository happens on Windows, so test files are checked out with CRLF line endings. Then, tests are run in Windows, Linux, and macOS against the repo that was cloned on Windows with CRLF:

Should the repo be renormalized when sent for each OS?

I still think Roslyn could be better here and detect the existing line endings, but still maybe something in CI here could be improved.

[captainsafia]

@Youssef1313 Thanks for looking into this?

Should the repo be renormalized when sent for each OS?

@dougbu @wtgodbe Do you know if it is possible to configure git to target new lines to each target machine? How do we handle tests that are sensitive to OS-specific line endings elsewhere (assuming those exist)?

I still think Roslyn could be better here and detect the existing line endings, but still maybe something in CI here could be improved.

Agree with this! I don't see any reason for Roslyn testing to be this sensitive when it comes to comparing line-endings, especially because AFAIK things like generators/analyzers aren't meant to be environment-specific. Is there an issue tracking this on the Roslyn side?

[Youssef1313]

I don't see any reason for Roslyn testing to be this sensitive when it comes to comparing line-endings

Testing library intentionally compares line endings to catch bugs. By my comment, I meant that I was expecting Roslyn itself to do best effort guessing line ending instead of using Environment.NewLine (when end_of_line isn't set), but that ended up being by-design.

[Youssef1313]

Do you know if it is possible to configure git to target new lines to each target machine?

I think this is already done in .gitattribtues, but this works on fresh clones.

If the repo is cloned on Ubuntu, then it's LF. If it's cloned on Windows, then it's CRLF. (which is the correct behavior)

What happens though is the repo being cloned on Windows, and then (as far as I understand) sent to other machines using SendHelixJob. I don't know how SendHelixJob works, but whatever the way is, it keeps line endings the same.

I'm not sure if this is something to improve on dotnet/arcade?

[wtgodbe]

@dougbu @wtgodbe Do you know if it is possible to configure git to target new lines to each target machine? How do we handle tests that are sensitive to OS-specific line endings elsewhere (assuming those exist)?

For tests like these that are dependent on OS line-ending, I think they aren't suited for Helix. I suggest skipping them on helix so that they run in the on-machine build jobs. You can do so like the following:

aspnetcore/src/DataProtection/Extensions/test/DataProtectionProviderTests.cs

Line 118 in e852e90

[SkipOnHelix("https://github.com/dotnet/aspnetcore/issues/6720", Queues = "All.OSX")]

In this case you'd skip it on all queues:

[SkipOnHelix("https://github.com/dotnet/aspnetcore/issues/<new issue>"]

[Youssef1313]

There are couple of solutions to resolve this without skipping the test.

• Replace all line endings with the Environment.NewLine both in input source and fixed source. (last commit does that for this specific test to see whether it passes CI)
• Introduce an editorconfig for codefix tests that sets end_of_line = lf, then the test infrastructure would replace \r\n with \n from inputs

Not sure which approach you'd prefer. Currently I'm testing with the first one being applied specifically to the single problematic test.

[captainsafia]

Introduce an editorconfig for codefix tests that sets end_of_line = lf, then the test infrastructure would replace \r\n with \n from inputs

How do you go about configuring the editorconfig? Will it be done in a way that conflicts with the repo-level one or does the configuration happen in source?

Replace all line endings with the Environment.NewLine both in input source and fixed source. (last commit does that for this specific test to see whether it passes CI)

I'm fine with doing this as part of the verifier.

[Youssef1313]

How do you go about configuring the editorconfig? Will it be done in a way that conflicts with the repo-level one or does the configuration happen in source?

In source, via TestState.AnalyzerConfigFiles. So the .editorconfig will simply be a "string" in the test that doesn't mess up with repo-level .editorconfig.

The current approach should be passing, but let me know if you prefer a different approach.

[captainsafia]

It seems like both approaches, when done properly, can be achieved by modifying the base Verifier class. I have a slight preference towards the editorconfig option since it seems clearer to document.

[Youssef1313]

It seems like both approaches, when done properly, can be achieved by modifying the base Verifier class. I have a slight preference towards the editorconfig option since it seems clearer to document.

Done in last commit. Let's see if it will pass.

[Youssef1313]

2023-01-19T08:23:04.0578516Z [xUnit.net 00:00:28.46]       content of '/0/Test0.cs' did not match. Diff shown with expected as baseline:
2023-01-19T08:23:04.0580088Z [xUnit.net 00:00:28.46]        using Microsoft.AspNetCore.Hosting;<LF>
2023-01-19T08:23:04.0581492Z [xUnit.net 00:00:28.46]       -using Microsoft.AspNetCore.Http;<LF>
2023-01-19T08:23:04.0582983Z [xUnit.net 00:00:28.46]       +using Microsoft.AspNetCore.Http;<CR><LF>
2023-01-19T08:23:04.0584410Z [xUnit.net 00:00:28.47]        using Microsoft.AspNetCore.Mvc;<LF>

I wasn't expecting that. This failure happens on Linux. Trying a different approach now to set the option directly in OptionSet

[Youssef1313]

Still doesn't work. I'll revert back to the initial approach 😕 .

[dougbu]

What is different about this one test❔ Numerous tests use CSharpCodeFixVerifier with strings from the source files.

I guess the question might be "Is this really the only test that adds new newlines when fixing C#❔" or perhaps "Does the analyzer here have a bug❔"

[Youssef1313]

@dougbu I think not so many tests out there are adding new usings/lines so that's why it wasn't caught.

The analyzer/codefix don't have bugs. The codefix is just adding an AddImportsAnnotation. Nothing more.

[wtgodbe]

There are couple of solutions to resolve this without skipping the test.

To be clear, my suggestion (SkipOnHelix) won't make it so the tests don't run - it will tell the test to run on the local machine rather than on Helix. All of our tests run on only one or the other.

[Youssef1313]

There are couple of solutions to resolve this without skipping the test.

To be clear, my suggestion (SkipOnHelix) won't make it so the tests don't run - it will tell the test to run on the local machine rather than on Helix. All of our tests run on only one or the other.

Thanks for the clarification!

@captainsafia would you like me to try this?

[dougbu]

Given we want to get rid of every test job except the Helix one, I'm a fan of the current approach. As long as the test area owners agree, let's do it this way.

[wtgodbe]

Given we want to get rid of every test job except the Helix one, I'm a fan of the current approach. As long as the test area owners agree, let's do it this way.

Is that tracked somewhere?

[dougbu]

Given we want to get rid of every test job except the Helix one, I'm a fan of the current approach. As long as the test area owners agree, let's do it this way.

Is that tracked somewhere?

I recall that @HaoK closed the overall issue for moving everything to Helix because we had individual issues for each area that seemed stuck on build agents. See #10424, #18799, #27609, #38818, and #39420 for example.

More generally, testing on build agents is a waste of resources and not where we want to be. So, let's not make the problem worse (even if most of the above issues have been considered low priority lately).

[captainsafia]

Thanks for the clarification!

@captainsafia would you like me to try this?

The editorconfig strategy didn't work, but does the ReplaceLineEndings one work? I would favor that being done here.

I also think addressing the issue well be prudent since we have more analyzers/codefixers coming down the pipe.

[Youssef1313]

The editorconfig strategy didn't work, but does the ReplaceLineEndings one work? I would favor that being done here.

I prefer moving this workaround to CSharpAnalyzerVerifier only when encountered in more tests.

[captainsafia]

The editorconfig strategy didn't work, but does the ReplaceLineEndings one work? I would favor that being done here.

I prefer moving this workaround to CSharpAnalyzerVerifier only when encountered in more tests.

Fair enough, in that case can you document what the workaround is for in case we run into it in future tests?

[captainsafia]

LGTM assuming we add a code comment clarifying what this is for

[captainsafia]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[Youssef1313]

/__w/1/s/.packages/microsoft.dotnet.arcade.sdk/8.0.0-beta.23063.7/tools/SourceBuild/SourceBuildArcadeBuild.targets(170,5): error MSB3073: The command "git submodule foreach --recursive '/__w/1/s/.packages/microsoft.dotnet.arcade.sdk/8.0.0-beta.23063.7/tools/SourceBuild/git-clone-to-dir.sh --source . --dest "/__w/1/s/artifacts/source-build/self/src/$sm_path" --clean'" exited with code 128. [/__w/1/s/.packages/microsoft.dotnet.arcade.sdk/8.0.0-beta.23063.7/tools/Build.proj]

@captainsafia Can you restart the source-build job please?

[captainsafia]

/__w/1/s/.packages/microsoft.dotnet.arcade.sdk/8.0.0-beta.23063.7/tools/SourceBuild/SourceBuildArcadeBuild.targets(170,5): error MSB3073: The command "git submodule foreach --recursive '/__w/1/s/.packages/microsoft.dotnet.arcade.sdk/8.0.0-beta.23063.7/tools/SourceBuild/git-clone-to-dir.sh --source . --dest "/__w/1/s/artifacts/source-build/self/src/$sm_path" --clean'" exited with code 128. [/__w/1/s/.packages/microsoft.dotnet.arcade.sdk/8.0.0-beta.23063.7/tools/Build.proj]

@captainsafia Can you restart the source-build job please?

There are some issues with source build that are pending a fix that are causing this failure. Rekicking the build is not likely to do anything.

@dotnet/aspnet-build How are we handling things in the meantime? Are we able to force-merge?

[wtgodbe]

The failure is due to the build agents, not anything in this repo: dotnet/source-build#3111. Some agents are new enough that they won't fail, some are not - in general you can ping me or doug to force-merge, or just re-run the source build job until it works (it's short). I'll force merge this one.