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

Add error check for skipped tests in merged groups #84284

Merged
merged 5 commits into from
Apr 4, 2023

Conversation

markples
Copy link
Contributor

@markples markples commented Apr 4, 2023

If I test is written in the old style (with a Main and OutputType==Exe without an attribute such as RequiresProcessIsolation) in a merged test group directory, it will be skipped. This change adds a check to detect those cases.

I have struggled with ways to automatically set OutputType. Directory.Build.props is too early (the test project file will override it). Directory.Build.targets is too late as the C# targets files will already have been processed and set other variables based on the value of OutputType. This Target doesn't execute until later, but since it is an error it doesn't matter how those additional properties were set.

Since this adds more boilerplate to each merged test directory, I created a src/tests/Directory.Merged.props to share all of that.

This catches 3 tests that aren't currently executing.

Unrelated:

  • Use GetPathOfFileAbove in nearby locations for Directory.Build.props chaining rather than specific paths
  • Fix two easy IL warnings that appeared in my local build of all tests

Resolves #84182

@markples markples added the test-enhancement Improvements of test source code label Apr 4, 2023
@markples markples added this to the 8.0.0 milestone Apr 4, 2023
@markples markples self-assigned this Apr 4, 2023
@ghost
Copy link

ghost commented Apr 4, 2023

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

Issue Details

If I test is written in the old style (with a Main and OutputType==Exe without an attribute such as RequiresProcessIsolation) in a merged test group directory, it will be skipped. This change adds a check to detect those cases.

I have struggled with ways to automatically set OutputType. Directory.Build.props is too early (the test project file will override it). Directory.Build.targets is too late as the C# targets files will already have been processed and set other variables based on the value of OutputType. This Target doesn't execute until later, but since it is an error it doesn't matter how those additional properties were set.

Since this adds more boilerplate to each merged test directory, I created a src/tests/Directory.Merged.props to share all of that.

This catches 3 tests that aren't currently executing.

Unrelated:

  • Use GetPathOfFileAbove in nearby locations for Directory.Build.props chaining rather than specific paths
  • Fix two easy IL warnings that appeared in my local build of all tests

Completes #84182

Author: markples
Assignees: markples
Labels:

test-enhancement, area-Infrastructure-coreclr

Milestone: 8.0.0

@markples
Copy link
Contributor Author

markples commented Apr 4, 2023

PTAL @trylek @jkoritzinsky
cc @dotnet/jit-contrib - I think the process of going through this (D.B.props/targets, etc.) may help with fixing BuildAsStandalone

@markples
Copy link
Contributor Author

markples commented Apr 4, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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 for cleaning this up and for fixing the previously overlooked cases!

<InMergedTestDirectory>true</InMergedTestDirectory>
<BuildAsStandalone Condition="'$(BuildAsStandalone)' == ''">false</BuildAsStandalone>

<AssemblyName Condition="'$(BuildAsStandalone)' != 'true'">$(MSBuildProjectName.Replace('_il_d', '').Replace('_il_r', ''))</AssemblyName>
Copy link
Member

Choose a reason for hiding this comment

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

IMO, it would be helpful to have some comments. Especially, to describe why we need this weird _il_d/_il_r handling.

@trylek
Copy link
Member

trylek commented Apr 4, 2023

Both errors you are seeing in the outerloop run should be fixed now by my and Michal's PR

#84268

I merged it in earlier today but after you triggered the outerloop runs.

@markples
Copy link
Contributor Author

markples commented Apr 4, 2023

Thank you all. @BruceForstall I will fold that into my next changes (to merge immediately since a new check and new tests are a race condition in the repo)

@markples markples merged commit 735ddea into dotnet:main Apr 4, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-coreclr test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merged test groups - check behavior when a test with incorrect settings is added to a merged directory
4 participants