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

Fix the test build error #86176

Merged
merged 3 commits into from
May 12, 2023
Merged

Fix the test build error #86176

merged 3 commits into from
May 12, 2023

Conversation

kunalspathak
Copy link
Member

@kunalspathak kunalspathak commented May 12, 2023

#86108 added a test that was failing the merge test checks.

https://dev.azure.com/dnceng-public/public/_build/results?buildId=272279&view=logs&j=c92f2c34-43c3-5f9c-356c-2e863ce9eb4e&t=1bb8c7f3-fff6-5069-c97d-796b6a1413a0&l=3934

/__w/1/s/src/tests/Directory.Build.targets(130,5): error : Tests in merged test directories should not set OutputType to Exe unless they are RequiresProcessIsolation, BuildAsStandalone, or IsMergedTestRunnerAssembly. [/__w/1/s/src/tests/JIT/opt/SSA/MemorySsa.csproj] [/__w/1/s/src/tests/build.proj]

@markples

@ghost ghost assigned kunalspathak May 12, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 12, 2023
@ghost
Copy link

ghost commented May 12, 2023

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

Issue Details

#86108 added a test that was failing the merge test checks.

@markples

Author: kunalspathak
Assignees: kunalspathak
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib

@kunalspathak
Copy link
Member Author

I am surprised the CI for original PR pass. @markples - do you know why?

@SingleAccretion
Copy link
Contributor

A PR race condition - #86108 was merged after the CI run but before merge.

It looks like the test is failing in outerloop/CG2 - can we disable it everywhere against #86112 (don't want to disable only on CG2 because it'll fail on NAOT and under collectible assemblies)?

(I was about to submit a change for that)

@kunalspathak
Copy link
Member Author

A PR race condition - #86108 was merged after the CI run but before merge.

Not sure if I follow. #86108 CI should have ran into this, right?

disable test

done.

@markples
Copy link
Member

If two PRs are open at the same time, they each test without the other. (I.e., we don't serialize testing/merging.)

I updated the .cs file to match. I plan on strengthening the checks in various ways to avoid mistakes, but also the copy/paste practice of writing tests is vulnerable while the merging process is going on, which will pass.

@kunalspathak
Copy link
Member Author

If two PRs are open at the same time,

Got it. What was the other PR you are referring to?

@markples
Copy link
Member

#85850 and #86108

@markples
Copy link
Member

Mine was open for a while because I hadn't gone through the "extra" test results and probably should have checked the history before merging. I just checked and there don't seem to be any other new tests in any of the directories that I just put into merged groups.

@kunalspathak
Copy link
Member Author

#85850 and #86108

That makes more sense now.

@markples markples merged commit 6023c87 into dotnet:main May 12, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants