-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Use a list for tests in src/tests/run.py #78463
Conversation
Otherwise we may problems due to multiple test assemblies having tests of the same name (e.g. due to _r, _ro versions of the same tests). Fix dotnet#78462
Tagging subscribers to this area: @hoyosjs Issue DetailsFix #78462
|
Not sure that this is the best fix. It works for other tests already without this change because they disambiguate via the My main concern is then whether we see the _r/_ro distinction anywhere in AzDO, but I think so, since the assembly name does include that. |
Can we change the test names and leave this as a check for future problems? |
It's like I said above – I don't think it matters. |
For example, here's one of these test failures: You can see both BEGIN EXECUTION
/datadisks/disk1/work/B5A409B3/p/corerun -p System.Reflection.Metadata.MetadataUpdater.IsSupported=false HardwareIntrinsics_r.dll ''
23:16:14.616 Running test: global::JIT.HardwareIntrinsics.Arm._AdvSimd.Arm64.Program.Abs_Vector128_Double()
Supported ISAs: Anyway, I will leave it up to @trylek and @davidwrighton whether they think this fix is ok, or if we should try to disambiguate the hardware intrinsics test names by assembly also. I'm not sure where to do the latter after #74886. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but feel free to wait for @trylek and/or @davidwrighton for more context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for fixing this!
I think this change is fine, but I'll admit I with the testresults.xml file had the distinction in it itself. |
Fix #78462