Approve console cmdlets tests by iSazonov · Pull Request #3101 · PowerShell/PowerShell

iSazonov · 2017-02-05T16:26:41Z

Main improvements refer to tests of the Write-Host cmdlet.
Original tests:

Slow because run external processes
Don't test colors and -NoNewLine in fact.

Done:

The original tests is preserved (deleted one as redundant) but marked
by 'Slow' tag. They is preserved because they actually check the output
on the work, not a test console.
Add negative color tests. (Code cover grow!)
Add tests based on TestHostCS. This test host has been refined so we
can see colors and a new line in output.
Add minor fixes for test modules loads.

Also I add support for Information stream. I originally planned to use
it but not actually used. However, I have left this as a useful addition
for future tests.
I wonder that a Write-Host console output is duplicated in Information
Stream - Is it by design? I left a debug print on this matter in the
test code.

Main improvements refer to tests of the Write-Host cmdlet. Original tests: 1. Slow because run external processes 2. Don't test colors and -NoNewLine in fact. 1. The original tests is preserved (deleted one as redundant) but marked by 'Slow' tag. They is preserved because they actually check the output on the work, not a test console. 2. Add negative color tests. (Code cover grow!) 3. Add tests based on TestHostCS. This test host has been refined so we can see colors and a new line in output. 4. Add minor fixes for test modules loads. Also I add support for Information stream. I originally planned to use it but not actually used. However, I have left this as a useful addition for future tests. I wonder that a Write-Host console output is duplicated in Information Stream - Is it by design? I left a debug print on this matter in the test code.

lzybkr · 2017-02-07T06:32:14Z

@@ -1,5 +1,14 @@
-Import-Module $PSScriptRoot\..\..\Common\Test.Helpers.psm1
-Import-Module $PSScriptRoot\..\..\Common\TestHostCS.psm1
+if ( ! (get-module -ea silentlycontinue Test.Helpers ))


Why? Import-Module does this already, right?

"Don't trust code patterns... Don't trust code patterns... Don't trust code patterns..." :-)
I am sorry but I cleaned up all files with bad pattern.

lzybkr · 2017-02-07T06:33:57Z


    if ( ! ("TestHost.TestHost" -as "type" )) {
-       $t = add-Type -pass $definition -ref $references
+       $t = add-Type -pass $definition -ref $references -WarningAction SilentlyContinue 


What warnings are being suppressed? I assume it's a C# warning - maybe disable with a pragma instead so we don't ignore future warnings that maybe shouldn't be ignored.

I check CI logs - no warnings. I see the warnings only locally.
So revert the change.

lzybkr · 2017-02-07T06:35:53Z

+        $ps.Runspace = $rs
+
+        $testHostCSData = @(
+            @{ Name = 'defaults';Command = "Write-Host a,b,c"; returnCount = 1; returnValue = @("White:Black:a b c:NewLine") }


I think this would be easier to read and review if each key/value pair was on a line by itself.

lzybkr · 2017-02-07T06:42:09Z

        public override void WriteLine(string value)
        {
-            Streams.ConsoleOutput.Add(value);
+            Streams.ConsoleOutput.Add("::"+value+":NewLine");


Why not use `n, or I guess since this is actually C#, \n? Then you don't need :NoNewline.

Idea is that we shouldn't mix "user" chars and "code" chars. If we would use "n" and value is "testn" then we will not be able to determine whether this character from the original string or generated by code (WriteLine).
We could use "test:n" or "test:\n" but if we look at the text of tests, it is easier to read NewLineandNoNewLine, especially since this is key information. We only need to avoid using ':' in value`. I believe that this is not a problem for test environment.

Is the preceding :: necessary?

This means that colors are not specified. We should use one template for all outputs. In the future someone can use "-Split" to analyze results regardless of which Write/WriteLine method is used.

If color is specified, I would use color:output as the expected string to test the result, so why :: is needed?

Here colors is not specified and it is Empty. I believe both output methods (with colors and w/o colors) should return strings in the same format. I would make "Empty:Empty:teststring:NewLine" but it is seems unneeded complicity.

I see your point now. It sounds over complex and useful at the same time 😄
I'm OK to have this change in TestHostCS.psm1, but could you please put more comments there for the Write/WriteLine APIs so that people can a bit easier to understand the behavior?

daxian-dbw · 2017-02-11T01:30:02Z

    }
    It "Out-Host writes to host output" {
        $stringToWrite = "thing to write"
+        $stringExpected = "::$($stringToWrite):NewLine"


Minor: the sub expression $() is not needed here.

Without $() it is not worked 😕

Ah, I see, the preceding colons.

No, it's the colon at :NewLine. Maybe use another special character as the delimiter?

This is debug output - we can use any perfectly visible symbol.
The only restriction is that not to use it in the source debug string ().
This character ':' is rarely used, so I suppose it is appropriate.
Or I missed something?

It's just less intuitive to construct the expected string if you are using a variable. But currently there is only one instance where $() is required. Let's keep it for now, and we can always change to something else if there are any complaints.

daxian-dbw · 2017-02-11T02:27:47Z


-    It "write-Host works with '<Name>' switch" -TestCases $testData -Pending:$IsOSX {
-        param($Command, $returnValue)
+        # I wonder that a console output is duplicated in Information Stream - Is it by design?


Yes, Write-Host creates information record. #2674 was a PR to speed up Write-Host by deferring initialization of its properties.

👍 Thanks!

daxian-dbw · 2017-02-11T02:28:45Z

+        # I wonder that a console output is duplicated in Information Stream - Is it by design?
+        Write-Host $th.ui.Streams.Information.Count
+        Write-Host $th.ui.Streams.Information[0]
+        Write-Host $th.ui.Streams.Information[1]


Maybe we can remove lines 83 to 85?

Currently we haven't test for the "by design" feature so we should convert the lines to real tests.

daxian-dbw · 2017-02-11T02:38:58Z

-        & $powershell -noprofile $Command | Should Be $returnValue
+        $result.Count | Should Be $returnCount
+        $result[0] | Should Be $returnValue[0]
+        $result[1] | Should Be $returnValue[1]


Maybe this should be changed to something similar to the following, in case a test data with more than 2 results gets added to $testHostCSData in future.

foreach ($i in 0..($returnCount - 1)) { $result[$i] | Should Be $returnValue[$i] }

What scenarios do you have in mind? It seems these tests verify everything we need and look simple and clear.

Thanks for making this change!
Yes, tests for Write-Host is thorough enough, but it's better to set a good pattern for other people to copy in their tests.

I totally agree!

daxian-dbw · 2017-02-11T04:01:19Z

@@ -1,18 +1,91 @@
-Describe "Write-Host DRT Unit Tests" -Tags "CI" {
+Describe "Write-Host DRT Unit Tests" -Tags "Slow","Feature" {


"Write-Host DRT Unit Tests"

Can you change this description to be more specific? Other tests here seem also DRT unit tests.

Fixed.

Maybe remove Feature tag?

I found this:

Microsoft.PowerShell.Management\Start-Process.Tests.ps1:1:Describe "Start-Process" -Tags @("CI","SLOW") {

So let's keep both slow and feature here.

daxian-dbw · 2017-02-11T04:02:14Z

        public override void WriteLine(string value)
        {
-            Streams.ConsoleOutput.Add(value);
+            Streams.ConsoleOutput.Add("::"+value+":NewLine");


Is the preceding :: necessary?

daxian-dbw · 2017-02-11T04:04:49Z

+        [array]$result = & $powershell -noprofile $Command
+        $result.Count | Should Be $returnCount
+        $result[0] | Should Be $returnValue[0]
+        $result[1] | Should Be $returnValue[1]


It's unlikely new test data will be aded to $testData, but I think it's better to use the same foreach ($i in 0..($returnCount - 1)) pattern (or something similar).

Suppress import-module warnings Rename Describes Add "-Object" test Add Stream.Information tests with TestHostCS

iSazonov · 2017-02-11T17:15:58Z

@PowerShellTeam We have a problem with CIs:

Failed to register repository 'mygetpsmodule'

daxian-dbw · 2017-02-13T22:20:16Z

@iSazonov weird, I do see two of your PRs ran into this break, but no others. The PR #3139 was opened an hour ago and the builds were successful. Is this an intermittent problem? I restarted the Linux CI and will keep an eye on it.

daxian-dbw · 2017-02-14T00:56:38Z

I re-ran the Linux Ci and this time there is no build break. It's possible that the myget feed we used for installing PowerShellGet and PackageManagement was having a hiccup 😦

daxian-dbw

LGTM

iSazonov · 2017-02-15T03:39:28Z

@daxian-dbw Thanks for code review!

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

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

lzybkr reviewed Feb 7, 2017

View reviewed changes

Fix after code review

ef44c7e

daxian-dbw reviewed Feb 11, 2017

View reviewed changes

daxian-dbw requested changes Feb 11, 2017

View reviewed changes

daxian-dbw reviewed Feb 11, 2017

View reviewed changes

daxian-dbw added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Feb 11, 2017

daxian-dbw self-assigned this Feb 11, 2017

Corrections after code review

c6dbf5a

Suppress import-module warnings Rename Describes Add "-Object" test Add Stream.Information tests with TestHostCS

PowerShellTeam added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Feb 13, 2017

Add checks for Streams.Information and add comments

63b2a7f

daxian-dbw approved these changes Feb 14, 2017

View reviewed changes

daxian-dbw merged commit fd70adb into PowerShell:master Feb 14, 2017

iSazonov deleted the add-color-parameter-tests-to-write-host branch February 16, 2017 03:42

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

		@@ -1,18 +1,91 @@
		Describe "Write-Host DRT Unit Tests" -Tags "CI" {
		Describe "Write-Host DRT Unit Tests" -Tags "Slow","Feature" {

Conversation

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Feb 13, 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!

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!

iSazonov commented Feb 11, 2017

Uh oh!

daxian-dbw commented Feb 13, 2017

Uh oh!

daxian-dbw commented Feb 14, 2017

Uh oh!

iSazonov commented Feb 5, 2017 •

edited

Loading

daxian-dbw Feb 13, 2017 •

edited

Loading