Is it really need?

No, removed.

Usually we use single quota.

Fixed

For discussion - We never do that but it seems we should mask test hooks by #if Debug.

Fixed

Ilya (@iSazonov) I think I need to revert this change. Wrapping that code in #if debug is causing the error condition to not occur, and thus the tests and CI to fail.

Dongbo Wang (@daxian-dbw) Jason Shirk (@lzybkr) Could you please clarify should we leave test codes in Release build?

We do leave test hooks in all builds because we primarily test release builds./

I would love to have those test hooks readonly in the release build, so they cannot be messed around to change powershell behavior. Problem is that some tests won't be able to run in a release build, which is undesired.

The same about single quota.
And please put comments on separate lines before codes.

Fixed

Please remove the comment or put on separate line before the code and expand it.

Fixed

Please remove the comment or put on separate line before the code and expand it.

Fixed

The same about single quota.

Fixed

Please use full pattern:

if (...)
{
    ...
}

Fixed

Typos - begin with capital letters.

Fixed

Why we skip Windows?

Because the error condition doesn't appear on Windows. Unlike Unix, deleting or renaming a file does not have an adverse effect on the enumeration---the file is listed in its original form.

So the test is ok on Windows too?

No. On Windows the test will fail because the enumeration will succeed with no errors emitted in the middle of the process, and $Error.Count will be zero.

We need to change those tests so that they work the same on all platforms.
In other words these tests now check the test hook not Get-ChildItem.

But the underlying behavior is not the same on all platforms. On Unix, if a file is deleted or renamed during enumeration over the result of DirectoryInfo.EnumerateFiles()an exception is thrown when attempting to access a property on the FileSystemInfo object. On Windows, no exception is thrown, the property is successfully accessed, and the enumeration goes happily on.

The test hook code does not force an error. It sets up the condition which may or may not trigger an error. If the same condition triggers an error on one platform but not on another, how are the tests for how the error is handled expected to behave the same on both platforms?

Sorry, I think I just realized what you meant. You want to change the test so that if ($IsWindows) the error count is zero, otherwise it is one. Is that right?

Original Issue:

Expected
Get-ChildItem should skip inaccessible paths (with error) and continue enumerating files and paths that are accessible.

Our tests must be simple as that expected: dir without throw return expected list (different on different platforms).

I hope this is what you're looking for. On Unix the tests check that the error was emitted and that the deleted/renamed item was left out of the list. On Windows the tests check that no error was emitted and that the list is complete.

I have also removed the #if DEBUG to allow the CI checks to succeed. I would rather the hook code be in debug-only, but the CI and Start-PSBuildbuilds seem to be release builds.

Here's a version in which the hook now only allow the test script to select what actions to take, but the hook code internally is hard-coded to use specific file names.

This test hook frightens me - please think through the security implications of this.

We can hard code the test file names and call InternalTestHooks.GciEnumerationAction to only enable/disable the hook.

We can leave the parameter as bool and define two variables internal static bool GciEnumerationActionDelete = false; and internal static bool GciEnumerationActionRename = false;

I feel like it would be better to not allow both to be set at the same time, which the string property accomplishes.

I talked about two variables - you can enable hooks at different times.

Changed. If both are set at the same time, only the delete will be done.

We need to try to avoid a match with real names. Maybe use Guid-based names for test file names?

Fixed. Using GUID-based names.

Why we use lower case for "_a" and below for "_c"?

The original filenames were a, B, c, D, E, and .F, in that letter casing, which appeared to have been a deliberate choice. To avoid disrupting any existing tests, I elected to keep the same letter casing in the GUID filenames.

It does look like I made a copy/paste error when I was generating GUID-based filenames. The filename for $Item_E should have begun with the letter/hex digit E. It now does.

The reason I used lower-case in the variable names was to call out that casing was significant.

Maybe "Should Be" ?

No doubt. Fixed.

Why not just use filesystemInfo.FullName as the fullName?

That was a safety choice. What actually triggers the error is invoking filesystemInfo.Attributes further down in the code. I wanted to avoid invoking any properties that might involve lazy evaluation, to avoid having the error occur while running test-hook code.

If we can be sure that using filesystemInfo.FullName won't cause the file system to be hit I would be happy to use it instead.

I changed it to filesystemInfo.FullName and the test passed on windows and OSX. I didn't try on Linux. Are you seeing issue with filesystemInfo.FullName?

I haven't seen an issue, no. I was just trying to avoid one. I'll go ahead and make the change to FullName.

Just tried on Ubuntu 16.04, filesystemInfo.FullName also works fine 😄

Fixed

ditto, maybe use filesystemInfo.FullName instead?

Fixed

It's recommended to avoid using == for string comparison. Please use string.Equals(a, b, InvariantCulture)

Fixed

ditto. I know this is for testing, but code pattern will be copied, so it's better to be consistent.

Fixed

Checking these 2 testing flags in the loop seems too expensive to me. How about do the testing trick before the foreach loop? Like:

var fullName = Path.Combine(directory.FullName, "c283d143-2116-4809-bf11-4f7d61613f92")
if (File.Exist(fullName))  // <--- even better, use our internal `ItemExist` method which calls native API
{
      File.Delete(fullName);
}

I'm concerned about hoisting this out of the loop. The point is to have a file go away while the enumeration is in progress. If we do the test outside the loop I don't think we've met the requirement---we've acquired the enumerator but have not yet started the enumeration. That's why we've chosen files that will not be the first in the enumeration.

It seems we can put this after IEnumerable<FileSystemInfo> sortedChildList = childList.OrderBy(c => c.Name, StringComparer.CurrentCultureIgnoreCase);

I'm not sure that solves the problem. Again, we have an enumerable object but enumeration doesn't start until the foreach.

I think we get already the list in foreach (IEnumerable<FileSystemInfo> childList in target) - so after that 'IEnumerable sortedChildList = childList.OrderBy(c => c.Name, StringComparer.CurrentCultureIgnoreCase);' we can safely delete/remove test file.

target is just a List of IEnumerable objects, containing only 1 or 2 entries---an IEnumerable for directory names and an IEnumerable for file names. I don't think the act of getting a reference to an enumerable object from a list causes enumeration to begin.

Again, we have an enumerable object but enumeration doesn't start until the foreach.

jeffbi This is a very good point. Let's keep it as is.

I catched an enumaration issue second time in last week - need to go to school. :-)

The last question is whether to make a test for such normal runtime error?

Same here - may be move this outside the loop. Let's keep it as is.

The comment should describe where these hooks are used, and used for what.

Fixed

The string.Equals check below uses StringComparison.InvariantCulture and this one uses StringComparison.Ordinal. It's better to be consistent.

D'oh! Leftover artifact, thanks for spotting that.

Fixed.