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

Added more verbose error message when calling method using reflection and target type does not match passed type #98129

Merged
merged 24 commits into from
Feb 16, 2024

Conversation

AndyBevan
Copy link
Contributor

Added more verbose error message when calling method using reflection and target type does not match passed type (fixes #97022)

… and target type does not match passed type (fixes dotnet#97022)
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 7, 2024
@ghost
Copy link

ghost commented Feb 7, 2024

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Added more verbose error message when calling method using reflection and target type does not match passed type (fixes #97022)

Author: AndyBevan
Assignees: -
Labels:

area-System.Reflection

Milestone: -

@AndyBevan
Copy link
Contributor Author

AndyBevan commented Feb 8, 2024

This fails on Linux but passes locally for me? I'm guessing I missed something somewhere because the test results I found are still showing the original error message. Hoping someone can offer me a pointer on what I missed. Thanks

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-98129-merge-bd36d088b19a4bdcad/System.Reflection.Tests/1/console.faa125c0.log?helixlogtype=result
[FAIL] System.Reflection.Tests.DefaultBinderTests.InvokeWithIncorrectTargetTypeThrowsCorrectException Assert.Equal() Failure: Strings differ ↓ (pos 7) Expected: "Object of type Test does not match target"··· Actual: "Object does not match target type." ↑ (pos 7) at System.Reflection!<BaseAddress>+0xb067e0 at System.Reflection.DynamicInvokeInfo.Invoke(Object, IntPtr, Object[], BinderBundle, Boolean) + 0xbc

@AndyBevan
Copy link
Contributor Author

Thanks @danmoseley for your PR review - I've now fixed the test failure. I did have a few questions related to working on the dotnet runtime in the related issue - #97022 (comment) - if you had time to help with answers that would be fantastic and it would help my understanding of how things fit together.

@danmoseley
Copy link
Member

Hi @AndyBevan this isn't my code area. @steveharter is it possible for you to give @AndyBevan the necessary pointers and help get this signed off?

@buyaa-n buyaa-n added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 14, 2024
… method is invoked using reflection, inline with other PR changes
@AndyBevan
Copy link
Contributor Author

I think everything is now resolved and tests now pass.

@danmoseley / @buyaa-n - would one of you be able to point me to instructions on how I would run the AOT version of the tests locally - ideally so I can debug them in Visual Studio - I had to make a lucky guess on the AOT version as I couldn't test it locally. I looked at what helix logs and I don't see a way to runt his locally >call RunTests.cmd --runtime-path C:\h\w\C3860A44\p

@buyaa-n
Copy link
Contributor

buyaa-n commented Feb 16, 2024

Tests did not run on CI because the branch was needed to be merged with latest, I clicked the merge button, now we will see if test fails

would one of you be able to point me to instructions on how I would run the AOT version of the tests locally

I have never run tests on AOT, according to the doc seems its need to be build with libs+clr.aot option and then run tests dotnet build -c Release -t:Test -p:TestNativeAot=true, though this would run all tests which would take too much time, better to move to the Runtime tests folder and run the test:
cd src\libraries\System.Runtime\tests
dotnet build /t:Test /p:TestNativeAot=true

@AndyBevan
Copy link
Contributor Author

Thanks so much @buyaa-n for doing that, also thanks for explaining to me how to run those AOT tests, this is a new area for me. Is there a way to run the AOT tests through visual studio?

Also - the tests seemed to fail for some javascript related problems. I'll update the branch with the latest from main and see if they pass on a second run.

@buyaa-n
Copy link
Contributor

buyaa-n commented Feb 16, 2024

Is there a way to run the AOT tests through visual studio?

Do not know for sure, do might help: https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/docs/debugging.md

Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

LGMT, thank you!

Failures unrelated and known

@buyaa-n buyaa-n merged commit 50c0153 into dotnet:main Feb 16, 2024
174 of 178 checks passed
@danmoseley
Copy link
Member

thanks @AndyBevan !

@github-actions github-actions bot locked and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection community-contribution Indicates that the PR has been added by a community member needs-author-action An issue or pull request that requires more info or actions from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve error reporting for dynamic method invocation via reflection
4 participants