Skip to content

Src gen: use GetBestTypeByMetadataName instead of GetTypeByMetadataName #59092

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.

Sign up for GitHub

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

Merged
steveharter merged 11 commits into from
Sep 29, 2021
Merged

Src gen: use GetBestTypeByMetadataName instead of GetTypeByMetadataName #59092

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
28db0c6
Use GetBestTypeByMetadataName instead of GetTypeByMetadataName
steveharter Sep 14, 2021
25dcab8
Fix merge issue
steveharter Sep 14, 2021
ff67850
Add unit test to Logging generator
steveharter Sep 15, 2021
a3e6eb1
Misc test cleanup
steveharter Sep 16, 2021
9c89de0
Attempt to address wasm test failure
steveharter Sep 16, 2021
46b3905
temp: remove ref to new code
steveharter Sep 18, 2021
051ed1b
temp: remove ref to new code (attempt 2)
steveharter Sep 20, 2021
4d6c7a3
Add back intended code
steveharter Sep 20, 2021
7a4eee9
Add test.proj ProjectExclusions
steveharter Sep 21, 2021
8e2ca47
Fix test exclusion path
steveharter Sep 23, 2021
e1ac8d8
Disambiguate Runtime-provided from Roslyn-provided helpers
steveharter Sep 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix test exclusion path
  • Loading branch information
@steveharter
steveharter committed Sep 27, 2021
commit 8e2ca4764538fd9409ef89edc0746597cca8ca63
6 changes: 3 additions & 3 deletions src/libraries/tests.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@
</ItemGroup>

<ItemGroup Condition="'$(TargetOS)' == 'Browser' and '$(BuildAOTTestsOnHelix)' == 'true' and '$(RunDisabledWasmTests)' != 'true' and '$(RunAOTCompilation)' == 'true'">
Copy link
Member

@lewing lewing Sep 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

you shouldn't need a new group for this, the exclusions 4 lines above should be enough but it looks like the paths are incorrect? I see

./Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.csproj
./Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/Microsoft.Extensions.Logging.Generators.Roslyn3.11.Tests.csproj

Copy link
Contributor Author

@steveharter steveharter Sep 21, 2021

Choose a reason for hiding this comment

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

The previous section (Condition="'$(TargetsMobile)' == 'true' and '$(TargetOS)' != 'Browser'") also excludes the same tests.

I obtained the conditions from #54833 which is per #51961 (but for System.Text.Json).

The paths appear correct to me and are consistent with other references to them.

Copy link
Member

@lewing lewing Sep 22, 2021

Choose a reason for hiding this comment

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

shouldn't the path start with Microsoft.Extensions.Logging.Abstractions\tests not Microsoft.Extensions.Logging\tests ?

Copy link
Contributor Author

@steveharter steveharter Sep 23, 2021

Choose a reason for hiding this comment

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

Yes thanks. Missed that.

@eerhardt it looks like the test exclusion added in 9035d94 has the wrong path. I left it as-is in this PR.

Copy link
Member

@lewing lewing Sep 23, 2021

Choose a reason for hiding this comment

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

since it's pointing at the wrong path and passing those old exclusions can probably be removed. I'll do it in a separate pr

Copy link
Contributor Author

@steveharter steveharter Sep 29, 2021

Choose a reason for hiding this comment

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

FWIW Eric fixed that in #59608

<!-- Crash on compilation: https://github.com/dotnet/runtime/issues/51961 -->
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Logging\tests\Microsoft.Extensions.Logging.Generators.Tests\Microsoft.Extensions.Logging.Generators.Roslyn3.11.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Logging\tests\Microsoft.Extensions.Logging.Generators.Tests\Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.csproj" />
<!-- Exceeds VM resources in CI on compilation: https://github.com/dotnet/runtime/issues/51961 -->
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Logging.Abstractions\tests\Microsoft.Extensions.Logging.Generators.Tests\Microsoft.Extensions.Logging.Generators.Roslyn3.11.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Logging.Abstractions\tests\Microsoft.Extensions.Logging.Generators.Tests\Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.csproj" />
</ItemGroup>

<!-- Projects that don't support code coverage measurement. -->
Expand Down