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

Convert JIT\Regression tests to merged test groups #83895

Merged
merged 33 commits into from
Mar 31, 2023
Merged

Conversation

markples
Copy link
Member

@markples markples commented Mar 24, 2023

The (admittedly long) list of commits are the logical steps involved in converting the tests. Each is relatively homogenous and easier to view than the overall diff. Automated commands are indicated by [command] notation as the start of the commit message.

Usability concerns are listed in #71732. Since JIT\Regression is actively updated, at least some minimum is required before merging (unlike something like il_conformance which is basically not modified). I've attempted to strike a balance there between "perfection" and actually getting this finished by marking a subset as necessary up-front.

These were missed by the previous ILTransform -n run.
They no longer clash because run renamed all of their conflicts,
but this renames them for consistency.
- Manually do missed -collapse-main-sig and fix placement of [Fact]
- Fix b89946 that referred to Main
- Fix Runtime_59444 reflection on Test*
@ghost
Copy link

ghost commented Mar 24, 2023

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

Issue Details

null

Author: markples
Assignees: markples
Labels:

area-System.Reflection.Metadata

Milestone: -

@markples markples added this to the 8.0.0 milestone Mar 24, 2023
@markples
Copy link
Member Author

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@markples
Copy link
Member Author

fyi @dotnet/jit-contrib - There is considerable demand to see the effects of merging asap, so I'm currently planning on merging this as soon as I have the last few tests (including new ones as they race against this PR) converted. This will require me monitoring new tests for some of the mistakes that we hope/plan to catch automatically.

@markples markples marked this pull request as ready for review March 29, 2023 05:16
@markples
Copy link
Member Author

@trylek #71732 has the (original) measurements on this, though of course the precise numbers change as more are added, deleted, or split apart. This PR will bring Brian/my efforts to ~2500 with ~1500 probably not far behind. This doesn't count (numbers via dir /s /b *proj | wc) Methodical (~2000), HardwareIntrinsics (161, but there's more going on there), or Loader\classloader\TypeGeneratorTests (~1500). These include priority=1 tests.

opportunity to remove the reflection and simply mark the callees as [Fact]s.

64883/76273 - Suppress the xunit warning so that the code can continue
using the normal (public-only) reflection search.
@markples
Copy link
Member Author

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@markples
Copy link
Member Author

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@markples
Copy link
Member Author

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@markples
Copy link
Member Author

@MichalStrehovsky I just hit this failure. This PR adds test merging to JIT\Regression so in theory should have no impact on nativeaot (product or tests). Is this something you've seen elsewhere? The closest that I could find was an old issue about OSX (#73299). This is x64 linux. Thanks for any insight.

Console log: 'nativeaot.SmokeTests' from job 54cb44e8-0e3d-48ea-ad42-fbb86e93557d workitem 5d0b02f1-7e34-4da5-9b13-e2a8618b4d50 (ubuntu.1804.amd64.open.rt) executed on machine a00AO7E running Linux-5.4.0-1103-azure-x86_64-with-Ubuntu-18.04-bionic

https://dev.azure.com/dnceng-public/public/_build/results?buildId=222351&view=logs&j=ddb4415b-4613-5bce-e937-0da25336f8b9&t=045c6e99-7b9a-570f-bc7f-14c39fc632af

https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-83895-merge-54cb44e80e3d48eaad/nativeaot.SmokeTests/1/console.7cfae149.log?helixlogtype=result

nativeaot/SmokeTests/DynamicGenerics/DynamicGenerics/DynamicGenerics.sh [FAIL]
  /datadisks/disk1/work/B60409DF/p/nativeaottest.sh: line 15:  8680 Aborted                 (core dumped) $_DebuggerFullPath $1/native/$exename "${@:3}"
      --------------------------------
      Running Test: ThreadLocalStatics.TLSTesting.ThreadLocalStatics_Test
      Expected: 100
      Actual: 134
      END EXECUTION - FAILED
      Test failed. Trying to see if dump file was created in /home/helixbot/dotnetbuild/dumps since 3/30/2023 8:17:34 AM
      Test Harness Exitcode is : 1
      To run the test:
      > set CORE_ROOT=/datadisks/disk1/work/B60409DF/p
      > /datadisks/disk1/work/B60409DF/w/AB650937/e/nativeaot/SmokeTests/DynamicGenerics/DynamicGenerics/DynamicGenerics.sh
      Expected: True
      Actual:   False
      Stack Trace:
           at nativeaot_SmokeTests._DynamicGenerics_DynamicGenerics_DynamicGenerics_._DynamicGenerics_DynamicGenerics_DynamicGenerics_sh()
           at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
           at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
      Output:
        /datadisks/disk1/work/B60409DF/p/nativeaottest.sh: line 15:  8680 Aborted                 (core dumped) $_DebuggerFullPath $1/native/$exename "${@:3}"

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky I just hit this failure. This PR adds test merging to JIT\Regression so in theory should have no impact on nativeaot (product or tests). Is this something you've seen elsewhere?

I'm on vacation. Cc @VSadov who just merged a TLS change yesterday. It's either a big coincidence, or related. We should be able to pull down the coredump with runfo.

@VSadov
Copy link
Member

VSadov commented Mar 31, 2023

I will take a look at the TLS issue. Could be related to the recent change. Does it fail on Windows as well?

@VSadov
Copy link
Member

VSadov commented Mar 31, 2023

I've obtained the dump and the test binary that crushed. So far not much luck.
The dump is not very useful.

(21e8.38cb): Signal SIGABRT code SI_TKILL (Sent by tkill system call) originating from PID 21e8

The stacks are missing any kind of symbolic info though. Hard to tell what happened.
The test binary runs and passes. I would not be surprised if the test will pass if reset. Could indeed be a coincidence.

I will look over the Unix parts of the TLS change. Maybe I can spot something in the code.

@markples
Copy link
Member Author

I only saw the Linux x64 failure (in CI), and as you suspected, it passed on retry.

@markples
Copy link
Member Author

close/reopen to poke the license/cla check

@markples markples closed this Mar 31, 2023
@markples markples reopened this Mar 31, 2023
@markples markples merged commit 52bbeeb into dotnet:main Mar 31, 2023
@BruceForstall
Copy link
Member

@markples There are new gcstress timeouts due to this change: https://dev.azure.com/dnceng-public/public/_build/results?buildId=224309&view=ms.vss-test-web.build-test-results-tab

(I kicked off a gcstress run immediately after you merged to check this)

@markples
Copy link
Member Author

Thanks @BruceForstall. I've attempted to fix these symptoms based on past failures in #84193. They may need additional striping. I think I will prepare a PR that simply sets RequiresProcessIsolation (run one test per process with the simpler wrapper).
This will avoid the additional churn that would occur by reverting/remerging.

@ghost ghost locked as resolved and limited conversation to collaborators May 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants