Skip to content

Changes to excluded test failures when using mono runtime.#33176

Merged
naricc merged 1 commit intodotnet:masterfrom
naricc:naricc/core-riuntime-tests
Mar 5, 2020
Merged

Changes to excluded test failures when using mono runtime.#33176
naricc merged 1 commit intodotnet:masterfrom
naricc:naricc/core-riuntime-tests

Conversation

@naricc
Copy link
Contributor

@naricc naricc commented Mar 4, 2020

These changes let us exclude known failures on the mono run time.

They also fix some things that prevented the tests from being built in debug.

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 mostly good, thanks for pulling this off! I would suggest a bit of mop-up work though:

  1. I'd like to better understand why we're changing the library configuration as it seems orthogonal to this task as such;

  2. Please consider following a similar pattern to we're using for other architecture, build flavor and OS-specific exclusions in issues.targets (see my comment regarding 'RuntimeType' variable);

  3. What about the Windows script (build-test.cmd), are you planning to include it in a subsequent commit? The mono-specific exclusions wouldn't work without it on Windows even though probably nothing tragic would happen, the variable would just be undefined and expanded to an empty string and the Mono-specific exclusions would just be ignored.

Thanks

Tomas

@naricc naricc force-pushed the naricc/core-riuntime-tests branch 3 times, most recently from a1a2d18 to 2a4ad79 Compare March 4, 2020 22:18
@naricc
Copy link
Contributor Author

naricc commented Mar 4, 2020

@trylek I planned to make the change to the .cmd file in a seperate PR. I don't actually have a development environment that can run .cmd files and didn't want to hold it up more while I set that up. Since it does not break anything, and the .sh file is probably more important for mono, that seems reasonable to me.

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.

LGTM, thank you!

@SamMonoRT
Copy link
Member

How do we exclude tests based on a particular mono config like for example running tests with interpreter or running with llvm ?

@trylek
Copy link
Member

trylek commented Mar 4, 2020

I guess that the existing mechanism can be further refined. The good news is that the issues.targets file kicks in after the actual managed test build, in the final phase of XUnit wrapper creation, so if there's for instance a reasonable local scenario for running multiple such mono configs in a row, it doesn't require rebuilding the tests. I treat Nathan's work as a pilot change in this direction that many others can follow along the same lines.

Copy link
Contributor

@jashook jashook left a comment

Choose a reason for hiding this comment

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

I personally would like to see the exclusions mapped into groups that are easy to understand which tests are disabled for a specifc runtime/arch/os.

As it is, it is hard to understand what os/arch the mono runtime tests are excluded under. My suggestion would be to add runtimename = coreclr and add this as check to each of the existing exclusion lists.

Basically, by adding mono, we are going to go from filtering on os/arch to runtime/os/arch and I would like to see that clearly annotated, as this file has been the source of weird problems in the past, where we suddenly stop building tests and we miss it for a while.

@trylek
Copy link
Member

trylek commented Mar 4, 2020

We are actually also filtering for Crossgen2, Simon just merged that in ;-).

@trylek
Copy link
Member

trylek commented Mar 4, 2020

For issues.targets in general I think you're right there are two smoking guns we should tackle somehow:

  1. Running some regular cleanup bot that would throw away tests blocked on issues that have been closed. This makes sure that a test doesn't remain disabled for another year just because the person who fixed the primary issue forgot, didn't realize or didn't even know he should remove an issues.targets exclusion.

  2. Getting rid of pseudo-items like "Needs triage" (I admit this change adds quite a few but we also have pre-existing ones) and replacing them with real actionable items conforming to (1).

My personal feeling is that we shouldn't insist on doing that as part of this PR to unblock the primary task of "making Mono tests green". As this is a new addition, I would initially ping @marek-safar for their action plan on triaging / addressing these issues and follow up with @jaredpar on the longer-term issues.targets policy. In practice it amounts to slightly more than 200 (or 10%) of Pri0 tests and it doesn't regress CoreCLR testing in any manner.

@trylek
Copy link
Member

trylek commented Mar 4, 2020

Actually, @naricc - can you confirm whether the exclusions cover Pri0 or Pri1 tests? I mean, if these are just Pri0 tests, we'll likely need about 800 more exclusions for the Pri1 tests, otherwise outerloop runs will remain red.

@naricc
Copy link
Contributor Author

naricc commented Mar 5, 2020

@trylek

This is just the priority 0 tests.

@marek-safar
Copy link
Contributor

marek-safar commented Mar 5, 2020

I see a few purposes of this "get something going" PR and it should go in with following follow up for @SamMonoRT to handle.

  • getting this hooked up into PRs testing (this included figuring out how change in coreclr folder will force mono retest)
  • create an issue for better categorization of issues in issues.targets (the current setup does not look very scalable to me).
  • Create issue for each failing tests on Mono configuration as we did for set/libraries tests
  • (edit from @akoeplinger) Remove RuntimeName property and use existing RuntimeFlavor instead.

@SamMonoRT
Copy link
Member

SamMonoRT commented Mar 5, 2020

I'll be creating 4 issues :

  1. Investigate and hook up running the with PRs/CI, making sure any change in CoreCLR or Mono triggers these test run on Mono
  2. Refactor the issues.target file to better categorize for different OS/architecture specific failures
  3. Remove RuntimeName property and use existing RuntimeFlavor instead.
  4. Update the issues.target file with Git hub links as new issues are created for each failing excluded test

Changes the way varaibles are propagated to issues.targets.

Tab/space change.

Reversed changes to test_dependencies, Diretory.Build.targets

Changed comparison.

Updated comment in issues.targets
@naricc naricc force-pushed the naricc/core-riuntime-tests branch from fcd2fca to 93fad7a Compare March 5, 2020 19:38
@naricc
Copy link
Contributor Author

naricc commented Mar 5, 2020

@jashook So from the discussion it sounds like we decided to address the issues in follow up PRs. So can you unblock this unless you have additonal issues?

I did update the comment in issues.target to make it more clear what we are attempting to exclude

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.

9 participants