Skip to content

Make Measure-Object property test independent of the filesystem #15879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
rjmholt merged 3 commits into from
Aug 9, 2021
Merged

Make Measure-Object property test independent of the filesystem #15879

rjmholt merged 3 commits into from
Aug 9, 2021

Conversation

@rjmholt
Copy link
Collaborator

@rjmholt rjmholt commented Aug 5, 2021

PR Summary

Fixes a recent CI failure where the expected and actual results of this test disagree because there's a directory in the $TestDrive.

PR Context

PR Checklist

@rjmholt rjmholt added the CL-Test Indicates that a PR should be marked as a test change in the Change Log label Aug 5, 2021
@ghost ghost assigned TravisEz13 Aug 5, 2021
@rjmholt rjmholt enabled auto-merge (squash) August 5, 2021 17:39
@rjmholt rjmholt disabled auto-merge August 5, 2021 18:53
@rjmholt rjmholt marked this pull request as draft August 5, 2021 18:53
@rjmholt rjmholt added the WG-Cmdlets-Utility cmdlets in the Microsoft.PowerShell.Utility module label Aug 5, 2021
@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Aug 6, 2021

Is this a race condition between getting the count of objects and than using measure-object? I'm not able to repro this with even an empty sub-folder?

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 7, 2021 •
edited
Loading

I can reproduce:

PS> $(Get-ChildItem C:\temp\ | Measure-Object -Property Length).Count
Measure-Object: Cannot process argument because the value of argument "Property" is not valid. Change the value of the "Property" argument and run the operation again.
0
PS > Get-ChildItem C:\temp\ -Recurse

    Directory: C:\temp

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d----          12.05.2021    17:22                52293_original

My current folder is not c:\temp\.

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Aug 7, 2021 •
edited
Loading

I see, this doesn't repro on macOS. Got a repro on my Windows machine. I think Measure-Object shouldn't fail here and should ignore it.

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 7, 2021

I don't understand the design:

PowerShell/src/Microsoft.PowerShell.Commands.Utility/commands/utility/Measure-Object.cs

Lines 820 to 827 in a4181ab

Statistics stat = _statistics[propertyName];
if (stat.count == 0 && Property != null)
{
// Why are there two different ids for this error?
string errorId = (IsMeasuringGeneric) ? "GenericMeasurePropertyNotFound" : "TextMeasurePropertyNotFound";
WritePropertyNotFoundError(propertyName, errorId);
continue;
}

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Aug 7, 2021

It looks like the design is to only return that error which is effectively terminating if the result is 0 and the property doesn't match. So if you have a folder that ONLY has an empty folder, then this will fail, but if it has any files it should succeed (which still doesn't make sense to me why the test fails unless it's a race condition).

Is there a repro where there are other items in the folder other than a single empty folder?

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 7, 2021 •
edited
Loading

It seems my bad. DirectoryInfo hasn't Length property. FileInfo has.

The PR is correct.

SteveL-MSFT
SteveL-MSFT previously requested changes Aug 7, 2021
View reviewed changes
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 7, 2021
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 9, 2021
@rjmholt rjmholt marked this pull request as ready for review August 9, 2021 20:36
@rjmholt rjmholt changed the title Make Measure-Object property test only look for files Make Measure-Object property test independent of the filesystem Aug 9, 2021
@rjmholt rjmholt requested review from SteveL-MSFT and iSazonov August 9, 2021 20:37
PaulHigin
PaulHigin approved these changes Aug 9, 2021
View reviewed changes
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rjmholt rjmholt dismissed SteveL-MSFT’s stale review August 9, 2021 22:26

Addressed the filesystem dependency issue

@rjmholt rjmholt merged commit 2f5150f into PowerShell:master Aug 9, 2021
@rjmholt rjmholt deleted the fix-measureobject-test branch August 9, 2021 22:27
@ghost
Copy link

ghost commented Aug 23, 2021

🎉v7.2.0-preview.9 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Sep 28, 2021

🎉v7.2.0-preview.10 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@iSazonov iSazonov Awaiting requested review from iSazonov

+1 more reviewer

@PaulHigin PaulHigin PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@TravisEz13 TravisEz13

Labels

CL-Test Indicates that a PR should be marked as a test change in the Change Log WG-Cmdlets-Utility cmdlets in the Microsoft.PowerShell.Utility module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@rjmholt @SteveL-MSFT @iSazonov @adityapatwardhan @PaulHigin @TravisEz13