search

aspnet / DataProtection

[Archived] Data Protection APIs for protecting and unprotecting data. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
130 stars 87 forks source link

Resolve continually flaky test in 2.1 by making it possible to mock the folder used for key storage #322

Closed natemcmaster closed 6 years ago

natemcmaster commented 6 years ago

Create an internal abstraction for finding the default directories for key storage. This allows us to run tests without squashing on keys on the developer machine. It also allows us to isolate test runs from reach other.

Fixes aspnet/Home#3034

natemcmaster commented 6 years ago

I can't fix the test without making a change to allow mocking a static method, so what do you propose we do?

Eilon commented 6 years ago

I just posted a comment to the issue thread: can we disable parallelization of these tests?

natemcmaster commented 6 years ago

The flakiness is not an issue with parallelization. It is a problem with bad machine state.

Eilon commented 6 years ago

If we disable parallelization can we have cleanup code that runs before/after each affected test so that they're guaranteed not to clobber over each other?

natemcmaster commented 6 years ago

It's not about parallelization. The global machine folder may be there for a number of reasons which disabling parallelization does not solve. when the global machine folder exists, the test attempts to avoid clobbering it by moving it temporarily. But moving it is what is causing problems. The location of the global machine folder is uninteresting in this test, which is why the fix for the flakiness is to use a different folder.

Eilon commented 6 years ago

What risk is there in disabling these tests for the patch?

natemcmaster commented 6 years ago

What risk is there in disabling these tests for the patch?

We lose regression coverage. If you'd rather risk that, then let's just get rid of the test.

Eilon commented 6 years ago

I think if we don't have a reasonable way of de-flakifying the test without product changes then we should disable the test.

natemcmaster commented 6 years ago

Per in person discussion, this is not okay because our default policy prohibits any change to anything in the product at all regardless of confidence that the changes are not going to break someone. The only exception is for changes which provide high value to customers, and flaky test fixes do not.