Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rebucket tests that fail with NativeAOT and fix some #71819

Merged
merged 10 commits into from
Jul 12, 2022

Conversation

MichalStrehovsky
Copy link
Member

I went through the "These tests don't build" uber bucket - pretty much all of those tests do actually build fine.

  • Some of them even run to completion without any failures. Unblocked those.
  • Fixed two that had problems:
    • System.Diagnostics.Debug.Tests was failing to build because the SingleFileTestRunner.cs references List and Dictionary and there's special magic needed in the project file for that to not to conflict between CoreLib and System.Collections.
    • System.Diagnostics.FileVersionInfo.Tests builds a test assembly that is not actually a part of the test, but a test asset. Reference it like that so that this strategy works with single file form factors. Fixes the test run.
  • Rebucketed the rest of the failing tests into those that we need to look into, and those that are not interesting.

Cc @dotnet/ilc-contrib

Actually failing build:
System.Diagnostics.Debug.Tests

Made fully passing:
System.Diagnostics.FileVersionInfo
All of these tests build just fine. Many of them also fully pass.
@ghost
Copy link

ghost commented Jul 8, 2022

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

I went through the "These tests don't build" uber bucket - pretty much all of those tests do actually build fine.

  • Some of them even run to completion without any failures. Unblocked those.
  • Fixed two that had problems:
    • System.Diagnostics.Debug.Tests was failing to build because the SingleFileTestRunner.cs references List and Dictionary and there's special magic needed in the project file for that to not to conflict between CoreLib and System.Collections.
    • System.Diagnostics.FileVersionInfo.Tests builds a test assembly that is not actually a part of the test, but a test asset. Reference it like that so that this strategy works with single file form factors. Fixes the test run.
  • Rebucketed the rest of the failing tests into those that we need to look into, and those that are not interesting.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: MichalStrehovsky
Labels:

area-System.Diagnostics

Milestone: -

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

Cc @radical @radekdoulik on commit ceb7fd1. I had to hack around WasmApp.targets treating all *.dll files in the app (including Content) as something that has meaning to the packaging. I would file an issue but I don't like filing issues that nobody will look at. Let me know if you would like an issue on it. It was running into Found identical vfs mappings for target path: System.Diagnostics.FileVersionInfo.TestAssembly.dll, source file: .../System.Diagnostics.FileVersionInfo.TestAssembly.dll without the workaround in the commit.

The test in question has a ProjectReference to a test assembly that is inspected using file IO APIs and it's not actually part of the app and should not be treated as such (should not be AOT compiled or whatever special we do to app assemblies - it's just a data file that happens to be an assembly).

@LakshanF
Copy link
Member

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@LakshanF
Copy link
Member

There are 1 failure each on Windows and Linux runs. I will disable these tests for now in tests.proj

@radical
Copy link
Member

radical commented Jul 11, 2022

Cc @radical @radekdoulik on commit ceb7fd1. I had to hack around WasmApp.targets treating all *.dll files in the app (including Content) as something that has meaning to the packaging. I would file an issue but I don't like filing issues that nobody will look at. Let me know if you would like an issue on it. It was running into Found identical vfs mappings for target path: System.Diagnostics.FileVersionInfo.TestAssembly.dll, source file: .../System.Diagnostics.FileVersionInfo.TestAssembly.dll without the workaround in the commit.

Currently, we are adding all the dll files from the publish directory. It's a known issue. They get added here:

<WasmAssembliesToBundle Include="$(PublishDir)\**\*.dll" Condition="'$(BuildAOTTestsOnHelix)' == 'true'" />
<WasmFilesToIncludeInFileSystem Include="@(ContentWithTargetPath)" />

And the content files are added for the VFS. Maybe we should remove those from WasmAssembliesToBundle.

@MichalStrehovsky
Copy link
Member Author

Thank you @radical! I've added reference to the issue to the workaround!

@LakshanF interesting, the System.Numerics.Vectors.Tests are not in the exclusions I'm rebucketing. This might be a recent regression. We might want to deal with that separately.

@radical
Copy link
Member

radical commented Jul 12, 2022

Wasm failures are fixed by #71976, which was just merged.

Copy link
Member

@LakshanF LakshanF left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for doing this and I will follow on the Linux failure separately

@LakshanF LakshanF merged commit 36208a5 into dotnet:main Jul 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants