Don't expand full drive globs with false condition #5669
Conversation
|
I have added an escape hatch. Setting |
| [InlineData(@"<i Condition='false' Include='/**/*.cs'/>")] | ||
| public void FullFileSystemScanGlobWithFalseCondition(string itemDefinition) | ||
| { | ||
| IList<ProjectItem> items = ObjectModelHelpers.GetItemsFromFragment(itemDefinition, allItems: false, ignoreCondition: true); |
There was a problem hiding this comment.
If we break this at some point in the future, it will seem like a hang rather than a failing test. Do you think it would be reasonable to add a timeout?
There was a problem hiding this comment.
I am not in favor of adding explicit timeouts to tests. My reasons are as follows:
-
[Philosophical] While some tests might seem more susceptible to hangs than others, it's as easy to break something by causing it to hang as it is to break it by causing it to return a wrong result. The test infra should have a reasonable default for the maximum duration, applied to all tests. Guessing how much time something should take is guaranteed to backfire - tests run on stronger/weaker hardware, in parallel, and so on. A lot of non-determinism in play for us to be able to claim that "this test should take a maximum of 10 seconds, otherwise it is a hang".
-
[Technical] The hang we're expecting in this test case would have the program spend an excessive amount of time in the
Projectconstructor. A long-running synchronous method is an API problem, let alone a long-running constructor. I guess that's an unfortunate legacy we have to live with. If I were to add a timeout, I'd run something like aThread.Sleep()in parallel, and if the sleep finishes before the test case does, I would:
a) Abort the hanging thread, risking that it leaves the disk or process in a corrupted or inconsistent state.
b) Let the thread run to completion but move on to executing other tests, marking this one failed.
In either case I wouldn't be able to trust the tests that execute after this because something may be amiss or interfering with normal test execution. For better or for worse, in general a hanging test case kills the entire test run.
There was a problem hiding this comment.
We do log all long test executions, though we don't currently have a hard timeout.
Co-authored-by: Forgind <Forgind@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Weird. Do you know if WaitOne works differently on Linux? From what I can tell, that test should never take noticeably more than 10 seconds. |
|
It could be hanging somewhere else. Here's a link to the failed build if you want to take a look: |
| [InlineData(@"<i Condition='false' Include='/**/*.cs'/>")] | ||
| public void FullFileSystemScanGlobWithFalseCondition(string itemDefinition) | ||
| { | ||
| IList<ProjectItem> items = ObjectModelHelpers.GetItemsFromFragment(itemDefinition, allItems: false, ignoreCondition: true); |
There was a problem hiding this comment.
We do log all long test executions, though we don't currently have a hard timeout.
Makes three PRs (#5669, #5625, and #5653) use Change Waves (#5710) to opt out as a unit. Also added a test to ensure that change waves worked properly for one of the three PRs and corrected a minor issue with tests for changes under a change wave that assume it is enabled when there exist tests that disable the relevant change wave without resetting it to empty at the end.
We often see hangs where MSBuild is enumerating the entire drive because conditions similar to this one are ignored at design time, most notably when loading projects in Visual Studio.
This PR addresses the problem by honoring the condition in cases where the evaluated glob file spec represents a full drive/filesystem walk, i.e. starts with
\**\or/**/.Fixes #4091 after adding the condition.