Export-Csv Test Improvements by Marusyk · Pull Request #5150 · PowerShell/PowerShell

Marusyk · 2017-10-17T21:48:41Z

iSazonov · 2017-10-18T05:28:06Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Export-Csv.Tests.ps1

@@ -3,71 +3,69 @@ Describe "Export-Csv" -Tags "CI" {
    $testCsv = Join-Path -Path $TestDrive -ChildPath "output.csv"


Please put the two lines in BeforeAll block.

iSazonov · 2017-10-18T05:29:19Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Export-Csv.Tests.ps1


    It "Should be able to be called without error" {
-	{ $testObject | Export-Csv $testCsv } | Should Not Throw
+        { $testObject | Export-Csv $testCsv } | Should Not Throw


Please add -ErrorAction Stop.

Export-Csv $testCsv -ErrorAction Stop

iSazonov · 2017-10-18T05:30:01Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Export-Csv.Tests.ps1


    It "Should throw if an output file isn't specified" {
-	{ $testObject | Export-Csv -ErrorAction SilentlyContinue } | Should Throw
+        { $testObject | Export-Csv -ErrorAction SilentlyContinue } | ShouldBeErrorId "CannotSpecifyPathAndLiteralPath,Microsoft.PowerShell.Commands.ExportCsvCommand"


Please use -ErrorAction Stop.

iSazonov · 2017-10-18T05:31:11Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Export-Csv.Tests.ps1

+        $piped = Get-Content $testCsv

-	$piped[0] | Should Match ".String"
+        $piped[0] | Should be "#TYPE System.String"


Please use:

$piped[0] | Should BeExactly "#TYPE System.String"

iSazonov · 2017-10-18T05:32:42Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Export-Csv.Tests.ps1

+        $switch = Get-Content $testCsv

-	$switch[0] | Should Match ".Object"
+        $switch[0] | Should Match ".Object"


Please use something like:

$piped[0] | Should BeExactly "#TYPE System.Object"

Done. But "#TYPE System.Object[]"

iSazonov · 2017-10-18T05:34:47Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Export-Csv.Tests.ps1

    It "Should be able to use the epcsv alias without error" {
-	{ $testObject | Export-Csv -Path $testCsv } | Should Not Throw
+        { $testObject | epcsv -Path $testCsv } | Should Not Throw
    }


Please remove the test at all - we have another file for alias tests.

iSazonov · 2017-10-18T05:35:26Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Export-Csv.Tests.ps1

+        { $testObject | epcsv -Path $testCsv } | Should Not Throw
    }

    It "Should have the same information when using the alias vs the cmdlet" {


Please remove the test at all - we have another file for alias tests.

iSazonov · 2017-10-18T09:06:59Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Export-Csv.Tests.ps1

+        }
    }

    It "Should have the same information when using the alias vs the cmdlet" {


Please remove the alias test.

iSazonov · 2017-10-18T09:09:11Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Export-Csv.Tests.ps1


-    It "Should be able to use the epcsv alias without error" {
-	{ $testObject | Export-Csv -Path $testCsv } | Should Not Throw
+        for ( $i = 0; $i -lt $testCsv.Length; $i++) {


I wonder why we use $testCsv.Length?
I'd expect $expected.Count.
Your thoughts?

I agree and think it should be $expected.Count. I have no clue how the length of the temporary file path string is relevant to anything in this test.

Is there even a need for looping logic here? would calling get-content once and perform 4 explicit tests be better? It seems like creating an array and looping through it (with a for loop no less) is just overkill.

I think we should stop on $expected.Count

iSazonov · 2017-10-18T11:03:07Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Export-Csv.Tests.ps1

-	# Clean up after yourself
-	Remove-Item $aliasObject -Force
+        for ( $i = 0; $i -lt $expected.Count; $i++) {
+         $(Get-Content $testCsv)[$i] | Should Be $expected[$i]


Please correct indentation.

markekraus

LGTM

iSazonov · 2017-10-23T08:51:36Z

@Marusyk Please resolve merge conflicts.

Marusyk · 2017-10-23T11:32:27Z

@iSazonov Done

markekraus · 2017-10-23T11:51:24Z

appveyor build fail due to #5201

markekraus · 2017-10-23T12:12:39Z

@Marusyk Right. that's what I said. the AppVeyor failure is due to the issue noted in #5201

daxian-dbw · 2017-10-23T23:16:30Z

#5118 has been merged, and I restarted AppVeyor CI for this PR.

markekraus

LGTM

iSazonov · 2017-10-24T04:38:21Z

@daxian-dbw Can we merge the PR?

daxian-dbw · 2017-10-25T22:08:48Z

@iSazonov Sorry that I didn't get a chance to review this change (too many notifications since having the code owner filter ...). Please proceed as you see appropriate :)

iSazonov · 2017-10-26T04:57:14Z

@Marusyk Thanks for your contribution! Welcome back with new PRs.

Export-Csv Test Improvements

e439eee

markekraus requested a review from daxian-dbw October 17, 2017 22:23

iSazonov reviewed Oct 18, 2017

View reviewed changes

iSazonov self-assigned this Oct 18, 2017

Fix PowerShell#5139

8500b41

iSazonov reviewed Oct 18, 2017

View reviewed changes

Fix PowerShell#5139

24a1b22

iSazonov reviewed Oct 18, 2017

View reviewed changes

Fix PowerShell#5139

953c20b

iSazonov approved these changes Oct 18, 2017

View reviewed changes

markekraus approved these changes Oct 18, 2017

View reviewed changes

Merge branch 'master' into master

13ec882

markekraus approved these changes Oct 23, 2017

View reviewed changes

iSazonov merged commit 282deb7 into PowerShell:master Oct 26, 2017

iSazonov added the Hacktoberfest Potential candidate to participate in Hacktoberfest label Oct 26, 2017

		@@ -3,71 +3,69 @@ Describe "Export-Csv" -Tags "CI" {
		$testCsv = Join-Path -Path $TestDrive -ChildPath "output.csv"

Conversation

Marusyk commented Oct 17, 2017 • edited by iSazonov 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!

Marusyk Oct 18, 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!

Marusyk Oct 18, 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!

markekraus left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Oct 23, 2017

Uh oh!

Marusyk commented Oct 23, 2017

Uh oh!

markekraus commented Oct 23, 2017

Uh oh!

markekraus commented Oct 23, 2017

Uh oh!

daxian-dbw commented Oct 23, 2017

Uh oh!

markekraus left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Oct 24, 2017

Uh oh!

daxian-dbw commented Oct 25, 2017

Uh oh!

iSazonov commented Oct 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Marusyk commented Oct 17, 2017 •

edited by iSazonov

Loading

Marusyk Oct 18, 2017 •

edited

Loading

Marusyk Oct 18, 2017 •

edited

Loading