Skip to content

Switch over JIT/Methodical tests to run using merged wrappers - #65597

Merged
trylek merged 3 commits into
dotnet:mainfrom
trylek:JIT-Methodical
Apr 18, 2022
Merged

Switch over JIT/Methodical tests to run using merged wrappers#65597
trylek merged 3 commits into
dotnet:mainfrom
trylek:JIT-Methodical

Conversation

@trylek

Copy link
Copy Markdown
Member

@trylektrylek added the area-Infrastructure-coreclr Only use for closed issues label Feb 18, 2022
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

fyi @jkoritzinsky

Author:trylek
Assignees:-
Labels:

area-Infrastructure-coreclr

Milestone:-

@ghostghost assigned trylekFeb 18, 2022
@trylek

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@trylek

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky

Copy link
Copy Markdown
Member

Two things that immediately popped out to me:

  1. We can remove the range_d.csproj and range_r.csproj projects as the tests they include are covered by Methodical_d.csproj and Methodical_r.csproj respectively.
  2. We need to update the generator to automatically exclude all <RequiresProcessIsolation>true</RequiresProcessIsolation> tests on any platforms where we can't launch new processes. Given the small test coverage loss there, I think that's okay (many of the tests that are going to have RequiresProcessIsolation set to true already don't run on mobile platforms due to other reasons).

@trylek

Copy link
Copy Markdown
MemberAuthor

@BruceForstall - once I pass initial outerloop validation on the change, I plan to start triggering Crossgen2 and stress runs to further validate the change. Please let me know which pipelines you find the most important to start with.

@trylek

Copy link
Copy Markdown
MemberAuthor

@jkoritzinsky - thanks for your immediate feedback. I believe my change should include removal of the wrappers under Array you previously added, if it doesn't, it will likely fail the build.

@trylek

Copy link
Copy Markdown
MemberAuthor

Hmm, in fact it won't fail the build, it only fails local test run because the Python code for producing the summary table hits a duplicate dictionary key. I'm not sure whether that's exercised in lab pipeline runs, I guess we'll see in an hour or two.

@jkoritzinsky

Copy link
Copy Markdown
Member

Yes, it looks like they have been removed. GitHub's diff was not showing it to me due to the number of changed files, but I validated in the GitHub Codespaces view that the Range merged wrappers are gone.

@trylek

Copy link
Copy Markdown
MemberAuthor

@jkoritzinsky - while this change is still WIP, I have tried to maintain some consistency w.r.t. its contents; I believe the particular bit we're discussing here should be covered in the commit

e4f2590

Thanks

Tomas

@BruceForstall

Copy link
Copy Markdown
Contributor

First would be runtime-coreclr jitstress. Most others do the same thing but set different environment variables, so maybe it's not necessary to run them all? Another: runtime-coreclr ilasm (round-trip tests). Also runtime-coreclr r2r and/or runtime-coreclr r2r-extra. And it might not hurt to run runtime-coreclr gcstress0x3-gcstress0xc (but unfortunately I don't think it's clean due to gcstress0x3 timeouts)

@trylek

Copy link
Copy Markdown
MemberAuthor

@jkoritzinsky - Hehe, I was curious why we're not observing the removal of the range_d and range_r projects you previously added. Turns out GitHub decided the newly added Methodical_* projects are their renames.

@jkoritzinsky

Copy link
Copy Markdown
Member

Ah Git rename detection. It always kicks in when you don't need it to and rarely kicks in when you actually want it to

@trylek

Copy link
Copy Markdown
MemberAuthor

One additional bit of debt in this change is the lack of issues.targets migration regarding the tests involved; I plan to work on that next.

@trylek

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Pull request contains merge conflicts.

@trylek

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@trylek
trylekforce-pushed the JIT-Methodical branch 2 times, most recently from ae7da2f to 45755b9CompareMarch 28, 2022 09:26
@trylek

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@trylek

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr jitstress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@trylek

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@trylek

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@trylek

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@trylek

Copy link
Copy Markdown
MemberAuthor

Based on Andrew's suggestion I have experimentally combined this change with a subtle tweak to src/tests/run.sh making event log opt-in instead of opt-out. The purpose of this experiment is to validate whether it fixes the failures seen in the r2r-extra pipeline on Unix. You can find more detail in the tracking issue #64263.

@trylek

Copy link
Copy Markdown
MemberAuthor

Hmm, in fact I don't think there's any chance this might work as the script is not actually exercised in the lab pipelines at all; instead I'll add targeted instrumentation letting us see at runtime whether the event log has been activated (in some other manner) directly in the offending runs.

@trylek

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@trylek

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@trylek

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@trylek

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@trylek

Copy link
Copy Markdown
MemberAuthor

I have verified that in the latest r2r-extra run I no longer see the hangs; some unrelated tests failed and two merged wrappers crashed on a runtime JIT assertion that is a known issue according to Bruce. I plan to merge this in after retesting unless some new catastrophic failure appears.

@trylek
trylek merged commit f865423 into dotnet:mainApr 18, 2022
@trylek
trylek deleted the JIT-Methodical branch April 18, 2022 08:59
@ghostghost locked as resolved and limited conversation to collaborators May 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Infrastructure-coreclrOnly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@trylek@jkoritzinsky@BruceForstall