Exclude failing CoreFx test System.Drawing.Tests.FontTests

[acmyu]

No description provided.

[A-And]

Thanks!
This is likely related to the failures in #18699.

[AaronRobinsonMSFT]

@A-And I don't know if excluding this is the right thing to do. Shouldn't instead the coreclr repo point at the right corefx version where that tests has been removed?

cc @stephentoub @danmosemsft

[A-And]

@AaronRobinsonMSFT - the CI job points to pre-built CoreFX binaries in Blob Storage, which we fetch and run the tests against. The exclusion list is meant to be temporary until we refresh the test binaries.

The full CoreFX tests fetched from the HEAD of the repo are still being run as a post-check-in job until we can update the test binaries automatically.

[AaronRobinsonMSFT]

@A-And Thanks! I didn't really know how that system works. This change makes more sense now. Really sorry about making a mess of this.

[A-And]

@AaronRobinsonMSFT Not at all - completely reasonable comment/assumption to make. :)

[stephentoub]

This should not be necessary. This test in corefx was fixed on Sunday morning. Doesn't coreclr's corefx tests CI run pull the current latest source from corefx? If so, this should not be failing and doesn't need to be disabled.

[stephentoub]

the CI job points to pre-built CoreFX binaries in Blob Storage, which we fetch and run the tests against.

Ignore my previous comment... I hadn't seen the intervening comments yet...

When did that change happen? Last I knew we were pulling the latest sources from corefx to do the CI runs. Unless we're frequently updating corefx binaries, this is going to lead to lots of failures as changes happen in coreclr that require coordinated changes in corefx.

[stephentoub]

Actually, @A-And, I'm confused by your comment:

the CI job points to pre-built CoreFX binaries in Blob Storage, which we fetch and run the tests against

Which binaries are being stored and fetched? Binaries for corefx product src, or binaries for corefx tests? If the latter, I understand; if the former, I don't understand how disabling this helps, nor why it's still an issue.

[A-And]

@stephentoub - Sorry, I was a bit unclear

CoreFX CI jobs in CoreCLR currently come in two flavors -

1. One run from tests\scripts\run-corefx-tests.py - this is the script which fetches the CoreFX repo at either the HEAD or a pre-determined commit. From what I understand, it was a bit too heavy and fragile
   to enable as a default CI job, which is why it only runs post-check-in

2. Another run from runtests.cmd - this is the one, which runs the default CI jobs on every PR. It builds a test host with the built CoreCLR components and downloads all CoreFX dependencies from the version specified in CoreCLR's dependencies - the actual product being built is up-to-date. In blob storage, we only store pre-built test assemblies. They are downloaded and run with the test host. The test that @acmyu is disabling is one of them.

The current setup for the default CI jobs is not ideal, but it allows us to run a test suite for every PR reasonably quickly.

[stephentoub]

Ok, thanks, so it is the test binaries that are being stored. When was that work done? I knew about (1); I had no idea about (2).

So, if we choose to make a change in coreclr that will break a corefx test, previously the developer would either a) make the change in coreclr and then fix the test as part of consuming the updated coreclr into corefx, or b) disable the test in corefx, make the change in coreclr, and then fix/reenable the test as part of consuming it into corefx. The test would generally only be broken or disabled for a matter of hours. Now what's the workflow?

[A-And]

It was enabled with #18365 .

For 2) - if there is a deliberate change, which causes a test failure, the fully-qualified name of the test method/class or entire namespace can be added to .\tests\CoreFX\TopN.CoreFX.x64.Windows.issues.json - that'll disable the test for all PR CoreFX test runs. Once a fix is in, we can remove the exclusion. The exclusion syntax includes a "reason" field for cases like this.

The motivation behind this was to keep quick changes like the one you described unilateral. The unfortunate side-effect of the change is that every refresh of the test binaries will need a quick audit of failing/passing tests.

[stephentoub]

The unfortunate side-effect of the change is that every refresh of the test binaries will need a quick audit of failing/passing tests.

It seems like it's more than that, or maybe I'm just misunderstanding. A developer wants to make a change in coreclr that'll end up breaking a corefx test (potentially multiple). So to get it in they need to make the change and disable the tests using this mechanism at the same time; ok. Then when that updated coreclr is consumed into corefx, the test in corefx is fixed to work correctly; ok. At that point, though, the developer needs to go back to coreclr and patch the test binaries so the test in coreclr can be re-enabled. How? Who has access? Does this mean that a Microsoft employee (and not other contributors) needs to be involved in this kind of change and actually build / push new test binaries? Or potentially even a specific one or more Microsoft employees that have write access to that blob storage?

Thanks.

[A-And]

Ah - I misunderstood.

In the case where both CoreFX and CoreCLR changes are necessary for a feature the plan is to leave the test disabled until we refresh the cached binaries. We haven't established a procedure for this yet, which is why the existing CoreFX jobs (i.e. 1) above ) are left as-is.

The assumption we made was that running with older binaries is fine for establishing a baseline.

As for the test binaries - the blob storage needs to be updated by someone with the Account key.

[A-And]

@dotnet-bot test Windows_NT x64 Checked CoreFX Tests please