Skip to content

Remove some manual test scaffolding #89653

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 31, 2023
Merged

Remove some manual test scaffolding #89653

merged 3 commits into from
Aug 31, 2023

Conversation

@markples
Copy link
Contributor

@markples markples commented Jul 28, 2023

These are either propagating unnecessary 100s or doing what the wrapper generator now does.

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 28, 2023
@ghost ghost assigned markples Jul 28, 2023
@ghost
Copy link

ghost commented Jul 28, 2023

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

Issue Details

These are either propagating unnecessary 100s or doing what the wrapper generator now does.

Author: markples
Assignees: markples
Labels:

area-CodeGen-coreclr
Milestone: -

@markples
Copy link
Contributor Author

markples commented Jul 28, 2023

These were found using a hacky analyzer that looked for N-1 of a test's returns to be if (expr == 100) return k; or some variations on that. The analyzer itself isn't appropriate for merging.

@markples markples marked this pull request as ready for review July 28, 2023 21:29
@markples
Copy link
Contributor Author

markples commented Jul 28, 2023

@TIHan PTAL

cc @trylek @jkoritzinsky @dotnet/jit-contrib No analyzer changes here - just test cleanup (deleting code and some Assert.Throws)

trylek
trylek approved these changes Jul 28, 2023
View reviewed changes
Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks Mark!

TIHan
TIHan approved these changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

Looks good to me as well!

@markples
Copy link
Contributor Author

markples commented Jul 28, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Jul 28, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@markples markples assigned trylek and TIHan and unassigned markples and trylek Aug 8, 2023
@markples
Copy link
Contributor Author

markples commented Aug 8, 2023

Actually, @TIHan, since you also reviewed this, could you please see this one through? The issue is just veriying through outerloop (since tests might be pri 1) and extra-platforms (don't want to break mono, for example), but those jobs are often noisy and verifying them isn't immediate.

@TIHan
Copy link
Contributor

TIHan commented Aug 9, 2023

@markples sure, I can try to get this through

@TIHan
Copy link
Contributor

TIHan commented Aug 9, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 9, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Aug 9, 2023
Closed
@trylek
Copy link
Member

trylek commented Aug 29, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 29, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@trylek
Copy link
Member

trylek commented Aug 29, 2023

Rebased against latest main, tested locally, fixed wrong merge with Mark's help, rerunning lab testing, planning to merge in assuming it passes.

@trylek
Copy link
Member

trylek commented Aug 29, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 29, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@markples
Copy link
Contributor Author

markples commented Aug 29, 2023

/azp run runtime-extra-platforms

@trylek
Copy link
Member

trylek commented Aug 29, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 29, 2023

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

azure-pipelines bot commented Aug 29, 2023

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Aug 30, 2023
Closed
Closed
Open
Open
Closed
@trylek
Copy link
Member

trylek commented Aug 31, 2023

Merging in as the linux x64 outerloop issues are known and understood (caused by running Crossgen2 in the lab under .NET 8 Preview 7 SDK, the bug is fixed in RC1 version of the SDK).

@trylek trylek merged commit c540d01 into dotnet:main Aug 31, 2023
@markples markples deleted the multiret branch September 28, 2023 23:32
@ghost ghost locked as resolved and limited conversation to collaborators Oct 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkoritzinsky jkoritzinsky jkoritzinsky approved these changes

@trylek trylek trylek approved these changes

+1 more reviewer

@TIHan TIHan TIHan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@TIHan TIHan

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@markples @TIHan @trylek @jkoritzinsky