Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

Add TargetArch (or similar) as a xunit.netcore.extensions #1979

Closed
sdmaclea opened this issue Mar 23, 2018 · 14 comments
Closed

Add TargetArch (or similar) as a xunit.netcore.extensions #1979

sdmaclea opened this issue Mar 23, 2018 · 14 comments

Comments

@sdmaclea
Copy link

It would be nice to have something similar to TestPlatforms for target arch x64,x86,arm,arm64

public enum TestPlatforms
{
Windows = 1,
Linux = 2,
OSX = 4,
FreeBSD = 8,
NetBSD = 16,
AnyUnix = FreeBSD | Linux | NetBSD | OSX,
Any = ~0
}

This would be intended to be used in corefx to extend the [ActiveIssue(int issue, TestPlatforms platforms)]test filtering to allow disabling for a specific architecture.

Current usage includes cases like this
[ActiveIssue(2743, TestPlatforms.AnyUnix & ~TestPlatforms.OSX)]

This would allow something like
[ActiveIssue(2743, TestPlatforms.AnyUnix, TestArch.ARM)]

Arm has a list of failing test https://github.com/dotnet/coreclr/issues/16001

It is currently disabling whole test suites.
https://github.com/dotnet/coreclr/blob/235a34673994560deb6011be08aff18b4399658b/tests/scripts/run-corefx-tests.bat#L20-L24

This would provide a mechanism for fime grain control.

x86 and arm64 will have similar issues. See https://github.com/dotnet/coreclr/issues/17020 for instance.

@BruceForstall @jkotas

@jkotas
Copy link
Member

jkotas commented Mar 23, 2018

@pgavlin tried to make this happen before he left: #1659

I am wondering what is the right level of metadata granularity to include inline with the corefx tests vs. in side-file.

The OS (Windows, Unix, ...) make sense to include inline with the corefx tests because of the OS differences are general concern for corefx repo.

The arch differences (arm, arm64, x86, ...) make less sense. And the specialized runtime configurations like GC stress, R2R, ... make even less sense to include inline with corefx tests because of those are not a concern for corefx repo. I think we may need a way to disable xunit tests via exclude file to make this work reasonably well, in addition to the existing capability to disable tests inline.

@sdmaclea
Copy link
Author

sdmaclea commented Mar 23, 2018

Maybe

-notrait category=failing_arm

Is sufficient

@sdmaclea
Copy link
Author

If the xunit.netconsole.exe could list the test class w/ method and consume a filtered list. We could add infrastructure to do this.

The only way I see to get a list of tests is to run them.

@sdmaclea
Copy link
Author

There are a lot of tests disable because of open issues. Per the developer guide they look like transient issues which are supposed to eventually be resolved. Seemed like this was how corefx was keeping CI green.

The exclusion list @BruceForstall used for arm is a bit heavy.

@sdmaclea
Copy link
Author

I'd love to enable corefx testing for arm64, but if it has to be green first... It may never happen. The exclusion lists are a reasonable start.

@sdmaclea
Copy link
Author

I looked briefly at #1659 It looks about exactly what I was describing.

The patch looks like it is in good shape, any reason it can not be merged?

@sdmaclea
Copy link
Author

Granularity of Metadata.

On linux-arm64, the Sytem.IO.Pipes test has 505 checks. One is failing when run in unpriviledged. I would rather not set up my CI with sudo, because it make my machines vulnerable. I would like to disable the single check, rather then the whole suite.

One option is to post process the testResults.xml files to mark the expected failures as if they were skipped..
That works as long as there isn't a hang or crash.

@jkotas
Copy link
Member

jkotas commented Mar 23, 2018

One is failing when run in unpriviledged. I would rather not set up my CI with sudo, because it make my machines vulnerable. I would like to disable the single check, rather then the whole suite.

This test should be disabled, fixed or moved to outter loop category on all Unixes, not just for arm64. We want the default corefx tests to pass in any environment: without admin rights, on non-English locales, in containers, etc.

Here are a few examples of similar fixes:

dotnet/corefx#28391
dotnet/corefx#28317

@BruceForstall
Copy link
Member

@sdmaclea I agree that the whole-assembly disabling mechanism I introduced for Windows ARM is too heavyweight (it was an expedient, really).

#1659 seems reasonable to me. Note, however, that Pat is gone, so it either needs to go in as-is, or someone needs to pick it up and update/PR/merge it.

In conversations with @jkotas, we've talked about a desire to have some kind of test corefx exclusion mechanism that would live as a checked-in file in the coreclr repro, to facilitate breaking changes, as well as our requirements around testing many JIT stress modes. I haven't thought much about what that should look like, though, because I have very little experience working with the corefx tests.

Maybe @weshaggard has some comments here.

@sdmaclea
Copy link
Author

sdmaclea commented Apr 4, 2018

One approach here might be to insert "ActiveIssue" when CoreCLR builds it.

A bash script like this could be stored in coreclr for each platform and would be simple enough.

#/bin/bash

# System.Diagnostics.Tests.ProcessTests.TestMainModule
sed -i \
  '/^ *public void TestMainModule() *$/i \
  [ActiveIssue("https://github.com/dotnet/coreclr/issues/17174")]' \
  src/System.Diagnostics.Process/tests/ProcessTests.cs

# System.Diagnostics.Tests.ProcessTests.TestId
sed -i \
  '/^ *public void TestId() *$/i \
  [ActiveIssue("https://github.com/dotnet/coreclr/issues/17174")]' \
  src/System.Diagnostics.Process/tests/ProcessTests.cs

@sdmaclea
Copy link
Author

sdmaclea commented Apr 4, 2018

Maybe a new category would be better [FailingCoreCLR] then the tests could selected by -notrait and -trait options

@jkotas
Copy link
Member

jkotas commented Apr 5, 2018

I think it is important for the CoreCLR-specific test exclusion mechanism to not require modifications of the test sources.

@BruceForstall
Copy link
Member

Note that with dotnet/coreclr#18365 we're getting closer to fine-grained exclusions. We still need to expand this support to all platforms and all stress modes.

@ViktorHofer
Copy link
Member

// Auto-generated message

Going forward, the .NET team is using https://github.com/dotnet/arcade to develop the code and issues formerly in this repository. Feel free to reopen/move this issue if it still applies to servicing branches in this repository, or source code which now resides in the Arcade repository.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants