Dongbo Wang (@daxian-dbw) Can you tag all the issues open for xunit tests in this PR?

Aditya Patwardhan (@adityapatwardhan) I tagged #8784 #8715. Please let me know if there are others.

I very wonder why we mask BUGs which are revealed by the tests?
Are we going to release 6.2 version that is subject to DoS attack by unprivileged user?
Are we going to release 6.2 version in which hosting applications will fall unpredictably in 9% of cases?

I expect that we fast fix the bugs:

• Use verbatim string literals for paths #8937 bug in test
• Reduce race condition probability for config file access #8779 - race conditions for scenarios with many runspaces/jobs
• follow PR for hosting scenarios (intra-process race conditions)

I very wonder why we mask BUGs which are revealed by the tests?

This is not hiding bugs, but to fix our test script. Blindly running all xUnit tests in parallel is just wrong. Please take a look at the original script in Start-PSxUnit before your changes in #8356, the tests are categorized into PSTests.Sequential and PSTests.Parallel namespaces and the original script ran the PSTests.Sequential tests sequentially, and run PSTests.Parallel tests in parallel.

The categorization was for a reason -- config files get created and deleted in test_PSConfiguration.cs, and that will definitely cause unusually frequent race conditions with tests in test_Runspace.cs. That's why they are categorized into PSTests.Sequential namespace. Using default values when failed to access the file as you suggested won't help here, because that will still make the setting values unpredictable and cause the tests to fail randomly -- a test might expect a specific value of a setting but it gets a default value because of the race condition.

I could make the test script behave like before -- running PSTests.Sequential namespace tests in sequence and PSTests.Parallel namespace tests in parallel, but I chose to not do that because:

1. we have to use the xunit config file to disable parallel, which make it cumbersome to run part of the tests sequentially and other part in parallel.
2. it makes writing xunit test harder by requiring the author to know which namespace to use for his/her tests.

Be noted that, fixing the test script doesn't mean we don't want to fix the race condition issue regard to processing the config file.

Are we going to release 6.2 version that is subject to DoS attack by unprivileged user?

Travis Plunk (@TravisEz13) and I chatted about this offline and we believe this is not a release blocker because it's already an existing issue.

Are we going to release 6.2 version in which hosting applications will fall unpredictably in 9% of cases?

The 9% failure was because of the frequent creation/deletion of the config files within test_PSConfiguration.cs, which would be really really rare to happen in real scenarios.

Be noted that, fixing the test script doesn't mean we don't want to fix the race condition issue regard to processing the config file.

I am 😄 that the team will keep it under control. Thanks!

One extra note that if I hadn’t made these tests parallel, we wouldn’t start fixing these race conditions. I do not know how now we can identify such problems (?)

One extra note that if I hadn’t made these tests parallel, we wouldn’t start fixing these race conditions. I do not know how now we can identify such problems (?)

The flag -p:ParallelizeTestCollections=false stopped working starting from some point, so luckily we will run into the same failures even without your changes.
Besides, the intra-process synchronization problem was a known issue. Please see the PR description in #5809 (I just updated it to add a bullet point about the case that config file is not accessible).

the intra-process synchronization problem was a known issue.

We have so many open issues that it is easy to lost/forget important ones. It is difficult to prioritize work. And it seems GiHub doesn't help well. :-(

The flag -p:ParallelizeTestCollections=false stopped working starting from some point, so luckily we will run into the same failures even without your changes.

This problem is too rarely raised and running the tests in parallel is useless locally. Can we do it on CI (maybe optional) to track the race conditions?

Can we do it on CI (maybe optional) to track the race conditions?

Like I said previously, fixing the race condition won't help to make our test reliable if they are running in parallel, because the setting values returned are not predictable. A test may expect a specific value for a setting but instead get back the default value due to race condition and thus would fail. It should be tracked by an issue.

This problem is too rarely raised and running the tests in parallel is useless locally.

It's rarely raised because it rarely happens in real scenarios, and that's likely why the this issue has not been treated with high priority.

It should be tracked by an issue.

In long term yes. For now my suggestion was to create optional CI job (like CodeFactor or Codacy) so that we can track the state of affairs until we find better solution.

It's rarely raised because it rarely happens in real scenarios, and that's likely why the this issue has not been treated with high priority.

Rather, because PowerShell is rarely used. If we imagine that it would be used in high-load application such as Microsoft SCOR we would catch a bunch of unpredictable crashes. Or in multi-user system like Misrosoft Remote Desktop any regular user would block all system scripts by locking the config file!

any regular user would block all system scripts by locking the config file!

You could do the same to any applications.
For example, hold the pwsh.exe by opening the file with [System.IO.FileShare]::None, and you won't be able to start the pwsh.exe anymore:

So, yes, it's technically a DoS attack, but if a user is able to do it (has the permission), then there's always a way for him/her to do it, no matter we fix the race condition regarding config.json or not.