Fix GetType() bad pattern and related issues in tests by iSazonov · Pull Request #3134 · PowerShell/PowerShell

iSazonov · 2017-02-12T12:55:11Z

$var.GetType() can raise an exception in tests so we should check $var
before make the call. A large part of the tests does not make this
check.
I start with searching ".GetType()" but discovered many related issues
in tests (reduntant and unneeded tests, "throw" bad pattens, bugs,
formattings (sorry!) and so on) - I had to fix them too.

According to our test roadmap, we must increase the code coverage from 45 up to 90% i.e. twice. Bad patterns and bad formatting will slow down our movement. I hope the team will support me in this PR.

daxian-dbw · 2017-02-13T22:32:05Z

pipe will unwrap the input collection, so this will become checking the type of individual elements instead of the collection as a whole. Better keep what it was in this case.

daxian-dbw · 2017-02-13T22:36:52Z

can this be $_.Exception | Should BeOfType $exception?

Don't work 😕

I see what is happening. $exception is point to internal types, and thus Pester cannot resolve. #Closed

daxian-dbw · 2017-02-13T22:39:44Z

`$result.GetType().BaseType | Should Be 'System.Object' seems not interesting, maybe remove it?

daxian-dbw · 2017-02-13T22:42:55Z

Is it failing? If it is we better have an issue tracking it.

OsLocalDateTime return empty to me. And the test is wrong. Yes, we should open a new Issue...

Yup, the test doesn't do anything about OsLocalDateTime. Please open an issue.

daxian-dbw · 2017-02-13T22:47:21Z

Can this line be removed?

I believe that the test is similar to the following after it. So it is better to leave.

daxian-dbw · 2017-02-13T23:03:21Z

if any of those aliases are removed, these tests can catch it. So maybe we should keep these 3 tests?

We have separate files with tests for aliases. If we need control a list of approved aliases we should make the test there. If it is we should open a new Issue.

Yes, please open an issue to verify the approved aliases exist in powershell. The test can be added to Microsoft.PowerShell.Utility\Default-Aliases.Tests.ps1.

daxian-dbw · 2017-02-13T23:06:12Z

indentation. there are similar indentation problems down in the changes.

I reformat the file.

daxian-dbw · 2017-02-13T23:19:07Z

Is it failing on Linux? I don't see this failure in our daily build ... Can you open an issue for it?

I only add the comment for you. I don't mark it as pending.
The code don't raise an exception as expected in the test (on windows). So we should open a new Issue.

Weird, it pass in daily build, but no exception is thrown when I run it locally.
@PaulHigin can you please take a look?

?? This test runs fine on Linux for me. The CI test failures only indicate that the class inheritance tests fail. Why do you think this test is failing?

I copy-paste the code

[System.Management.Automation.Runspaces.SSHConnectionInfo]::new( [System.Management.Automation.Internal.AutomationNull]::Value, "localhost", [System.Management.Automation.Internal.AutomationNull]::Value)

to interactive session (on Windows) and it don't raise exception.

Oh I see now. Yes, this test should be removed because the code now gets the current user name if no user name is specified. Good find.

Clear. Thanks! I'll remove the test.

@PaulHigin Why doesn't it fail in our daily test run then? There is no current user name when running daily test pass in AppVeyor builder?

Not sure. But the test is incorrect (no longer needed) because a null UserName parameter is now allowed, and the test should be removed.

The test is removed.

daxian-dbw · 2017-02-13T23:21:19Z

You use $result | Should BeOfType in some places, and $result | Should Not BeNullOrEmpty then $result.Exception.GetType | Should Be in some other places. Why don't you unify them with a single pattern?

Sometimes it is clear from above code that '$result' is not empty. Ex. $result.Count | Should Be 2

I see, if it's obvious $result is not null, you will go with $result | should BeOfType. If not, you will check $result | Should Not BeNullOrEmpty first.
But after checking "Not BeNullOrEmpty", why not also use $result | should BeOfType? I see in some places you just continue to use $result.GetType() | Should Be.

daxian-dbw · 2017-02-14T17:19:48Z

$ev will always be an arraylist, but it might be empty. I think here you want to test $ev[0] | Should Not BeNullOrEmpty.

We should not trust a cmdlet which we check.
If $ev != Null then $ev[0] never raise.
So I suppose $ev | Should Not BeNullOrEmpty is correct.

Understood. #Closed

JamesWTruher · 2017-02-14T19:14:26Z

this extra test doesn't really get you that much. If $HtAst is null, or if GetType is null, or $HtAst.SafeGetValue() doesn't return a hashtable, the test will fail regardless.
Doesn't just $HtAst.SafeGetValue() | should beoftype hashtable suffice?
most of the validating that the return value isn't null doesn't provide that much value (as the test will fail when the method is called), and in the case that it is not a failure, only adds time to the test.

I removed "GetType().Name" However, previously we need to check that $HtAst is not Null to exclude exception.

$var.GetType() can raise an exception in tests so we should check $var before make the call. A large part of the tests does not make this check. I start with searching ".GetType()" but discovered many related issues in tests (reduntant and unneeded tests, "throw" bad pattens, bugs, formattings (sorry!) and so on) - I had to fix them too.

Removed 'GetType().Name' patterns.

iSazonov · 2017-02-15T14:34:10Z

Sorry I accidentally made a rebase 😕

iSazonov · 2017-02-16T03:09:09Z

@daxian-dbw Thanks for the very fast code review! 👍

msftclas added the cla-already-signed label Feb 12, 2017

PowerShellTeam added the Review - Needed The PR is being reviewed label Feb 13, 2017

joeyaiello requested a review from JamesWTruher February 13, 2017 21:39

daxian-dbw requested changes Feb 13, 2017

View reviewed changes

daxian-dbw reviewed Feb 14, 2017

View reviewed changes

JamesWTruher reviewed Feb 14, 2017

View reviewed changes

iSazonov added 2 commits February 15, 2017 11:55

Fix after code review

3d2d710

iSazonov force-pushed the gettypenullreference branch 2 times, most recently from 2ca9f96 to c442923 Compare February 15, 2017 13:35

Second wave of migration GetType() -> BeOfType

28eee62

Removed 'GetType().Name' patterns.

iSazonov force-pushed the gettypenullreference branch from c442923 to 28eee62 Compare February 15, 2017 14:10

daxian-dbw approved these changes Feb 15, 2017

View reviewed changes

daxian-dbw merged commit 409ab74 into PowerShell:master Feb 16, 2017

iSazonov deleted the gettypenullreference branch February 16, 2017 03:12

iSazonov removed the Review - Needed The PR is being reviewed label Mar 27, 2017

Conversation

iSazonov commented Feb 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesWTruher Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

daxian-dbw Feb 14, 2017 •

edited

Loading

daxian-dbw Feb 14, 2017 •

edited

Loading

JamesWTruher Feb 14, 2017 •

edited

Loading