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

Disable tests failing apparently due to coverage outage regressions #35860

Closed
wants to merge 5 commits into from

Conversation

sdmaclea
Copy link
Contributor

@sdmaclea sdmaclea commented May 5, 2020

The first 2 patches are part of #35783. This is intended to merge after that patch.

sdmaclea added 5 commits May 5, 2020 17:32
Some tests are configured to run on a targets with
specific charecteristics.

Use issues.targets to conditionally disable tests
not intended to run on all targets.
@trylek
Copy link
Member

trylek commented May 5, 2020

Hmm, the issues.targets bloat looks somewhat worrisome to me; I've always thought that issues.targets should cater for "fixable bugs" (regressions, features soon to be implemented or not yet 100% functional etc.) as opposed to "known limitations" that should be ideally expressed in the project files. Why can't we just state that a particular test is CLRTestTargetUnspported (e.g. on Linux or on ARM64) without having to duplicate this information in the issues.targets file? Or am I reading it all wrong?

@jashook
Copy link
Contributor

jashook commented May 5, 2020

I had suggested it as a location for now then we could clean it up later. However, as the change is it is a huge amount of specific include/excludes that seems like they are easy to miss.

@sdmaclea
Copy link
Contributor Author

sdmaclea commented May 5, 2020

as opposed to "known limitations"

There are a lot of "known limitations" in the current issues. For instance vararg tests.

Why can't we just state that a particular test is CLRTestTargetUnspported (e.g. on Linux or on ARM64) without having to duplicate this information in the issues.targets file? Or am I reading it all wrong?

I believe it could be possible, but the urgency around #35783 made it so this was the best short term solution.

I am happy to get this where we want when we have less urgency.

they are easy to miss.

If they are not listed they will currently fail in test runs.
If they are extra ... someone had to put them there.

@sdmaclea
Copy link
Contributor Author

sdmaclea commented May 5, 2020

For what it is worth I used a bash script to generate the excludes. So it was fairly easy to make exhaustive for the current tip at the time...

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.

As I mentioned in Teams, I think this is fine to unblock the current situation but I believe it shouldn't stay that way forever. Longer term, I believe we should strive to make a clear distinction between "fixable bugs" (regressions, remaining bugs in new features) - that's what I believe "issues.targets" is for. If COM tests don't work on Linux or some Intel intrinsic tests don't work on ARM64, that sounds like "known limitations" to me and we should be tagging these somehow in the test projects, not in "issues.targets". That doesn't say such limitations must be set in stone - maybe one day there's going to be COM on Linux - but such a thing is not about "triaging and fixing bugs", it's about making design decisions. With this reservation I'm approving the change as a short-term measure to consolidate our builds.

@sdmaclea
Copy link
Contributor Author

sdmaclea commented May 6, 2020

Fixed in another PR...

@sdmaclea sdmaclea closed this May 6, 2020
@sdmaclea sdmaclea deleted the CoverageOutageRegressions branch September 26, 2020 16:52
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants