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

Fix il verification tests #39538

Merged
merged 3 commits into from
Jul 18, 2020
Merged

Conversation

davidwrighton
Copy link
Member

  • Re-enable the test project build
  • Change string resources in type system to directly use ResourceManager instead of SR abstraction which is unavaliable in the test build tree
  • Add test to verify that all ExceptionStringID values have associated strings
  • Use ToolsCommonPath property to successfully reference the correct resource file

Fixes #39504

- Re-enable the test project build
- Change string resources in type system to directly use ResourceManager instead of SR abstraction which is unavaliable in the test build tree
- Add test to verify that all ExceptionStringID values have associated strings
- Use ToolsCommonPath property to successfully reference the correct resource file
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@davidwrighton
Copy link
Member Author

@dotnet/crossgen-contrib
@sandreenko

@davidwrighton davidwrighton added area-crossgen2-coreclr area-Tools-ILVerification Issues related to ilverify tool and IL verification in general labels Jul 17, 2020
@davidwrighton
Copy link
Member Author

As a side benefit I tweaked the source so that it uses Path.Combine to handle directory work, so this test now passes on Linux as well as on Windows.

@@ -13,6 +16,9 @@ public abstract class TypeSystemException : Exception
{
private string[] _arguments;

private static Lazy<ResourceManager> s_stringResourceManager =
Copy link
Member

@MichalStrehovsky MichalStrehovsky Jul 17, 2020

Choose a reason for hiding this comment

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

Would it be possible to pull this out of TypeSystemException.cs into a separate file? This will not work when the type system is used as a runtime type system (we need to be able to get rid of ResourceManager in the UseSystemResourceKeys mode and that one relies on the SR type).

I think now I'm remembering why these were [TEMPORARY EXCEPTION MESSAGE] for such a long time...

Also the id.ToString() will not work because the enum is reflection blocked in the runtime type system.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'll pull these out into a separate file. Good point on the CoreRT impact. I had forgotten some of these details. Someone in the CoreRT repo will need to implement the resource lookup function in whatever way actually makes sense at runtime.

@davidwrighton davidwrighton requested a review from janvorli July 17, 2020 21:34
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@jkotas jkotas merged commit bb27de1 into dotnet:master Jul 18, 2020
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
* Revert "delete test project. (dotnet#39501)"

This reverts commit ecb231c.

* Fix ILVerification tests
- Re-enable the test project build
- Change string resources in type system to directly use ResourceManager instead of SR abstraction which is unavaliable in the test build tree
- Add test to verify that all ExceptionStringID values have associated strings
- Use ToolsCommonPath property to successfully reference the correct resource file

* Address feedback and allow the strings to be handled with a runtime implementation
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
@davidwrighton davidwrighton deleted the fix_il_verification_tests branch April 20, 2021 17:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-crossgen2-coreclr area-Tools-ILVerification Issues related to ilverify tool and IL verification in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JitStress job is failing to build ILVerification.Tests.csproj
6 participants