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.

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 test groups - check behavior when a test with incorrect settings is added to a merged directory #84182

Open
Tracked by #71732
markples opened this issue Mar 31, 2023 · 4 comments · Fixed by #84284
Open
Tracked by #71732

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

markples opened this issue Mar 31, 2023 · 4 comments · Fixed by #84284
Assignees
@markples
Labels
area-Infrastructure-coreclr Priority:3 Work that is nice to have test-enhancement Improvements of test source code

Comments

@markples
Copy link
Member

markples commented Mar 31, 2023 •
edited
Loading

The main example here is that we remove the OutputType==Exe setting. What happens if a new test is added with this setting?
@markples markples mentioned this issue Mar 31, 2023
Open
33 tasks
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Mar 31, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 31, 2023
@markples markples changed the title (R) What happens if someone adds a non-merged test to a merged test directory? (mainly, make sure that we avoid silently ignoring new tests) Merged test groups - check behavior when a test with incorrect settings is added to a merged directory Mar 31, 2023
@markples markples self-assigned this Mar 31, 2023
@ghost
Copy link

ghost commented Mar 31, 2023

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

Issue Details

The main example here is that we remove the OutputType==Exe setting. What happens if a new test is added with this setting?

Author: markples
Assignees: markples
Labels:

area-Infrastructure, untriaged, needs-area-label
Milestone: -

@markples markples added test-enhancement Improvements of test source code and removed untriaged New issue has not been triaged by the area owner needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners area-Infrastructure labels Mar 31, 2023
@ghost
Copy link

ghost commented Apr 2, 2023

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

Issue Details

The main example here is that we remove the OutputType==Exe setting. What happens if a new test is added with this setting?

Author: markples
Assignees: markples
Labels:

test-enhancement, area-Infrastructure-coreclr
Milestone: -

@markples
Copy link
Member Author

markples commented Apr 4, 2023

Such tests appear to be built but not run during CI, etc.

@markples markples mentioned this issue Apr 4, 2023
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 4, 2023
markples added a commit that referenced this issue Apr 4, 2023
@markples
Add error check for skipped tests in merged groups (#84284)
735ddea 
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
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 4, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 4, 2023
@markples
Copy link
Member Author

This needs to be strengthened. I saw an incorrect test that made it past the check in #84284 though can't remember it at the moment. I plan an audit of new tests to see what's been happening and will see it then.

@markples markples reopened this May 18, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 18, 2023
@markples markples added Priority:3 Work that is nice to have and removed untriaged New issue has not been triaged by the area owner labels Jun 6, 2023
@dotnet dotnet unlocked this conversation Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@markples markples

Labels
area-Infrastructure-coreclr Priority:3 Work that is nice to have test-enhancement Improvements of test source code
Projects
Runtime Infra
Status: No status
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add error check for skipped tests in merged groups markples/runtime
2 participants
@jeffschwMSFT @markples