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

Roman Marusyk (Marusyk) · 2017-10-17T21:48:41Z

Ilya (iSazonov) · 2017-10-18T05:28:06Z

@@ -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.

Ilya (iSazonov) · 2017-10-18T05:29:19Z


    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

Ilya (iSazonov) · 2017-10-18T05:30:01Z


    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.

Ilya (iSazonov) · 2017-10-18T05:31:11Z

+        $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"

Ilya (iSazonov) · 2017-10-18T05:32:42Z

+        $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[]"

Ilya (iSazonov) · 2017-10-18T05:34:47Z

    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.

Ilya (iSazonov) · 2017-10-18T05:35:26Z

+        { $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.

Ilya (iSazonov) · 2017-10-18T09:06:59Z

+        }
    }

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


Please remove the alias test.

Ilya (iSazonov) · 2017-10-18T09:09:11Z


-    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

Ilya (iSazonov) · 2017-10-18T11:03:07Z

-	# 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.

Mark Kraus (markekraus)

LGTM

Ilya (iSazonov) · 2017-10-23T08:51:36Z

Roman Marusyk (@Marusyk) Please resolve merge conflicts.

Roman Marusyk (Marusyk) · 2017-10-23T11:32:27Z

Ilya (@iSazonov) Done

Mark Kraus (markekraus) · 2017-10-23T11:51:24Z

appveyor build fail due to #5201

Mark Kraus (markekraus) · 2017-10-23T12:12:39Z

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

Dongbo Wang (daxian-dbw) · 2017-10-23T23:16:30Z

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

Mark Kraus (markekraus)

LGTM

Ilya (iSazonov) · 2017-10-24T04:38:21Z

Dongbo Wang (@daxian-dbw) Can we merge the PR?

Dongbo Wang (daxian-dbw) · 2017-10-25T22:08:48Z

Ilya (@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 :)

Ilya (iSazonov) · 2017-10-26T04:57:14Z

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

Export-Csv Test Improvements

e439eee

Mark Kraus (markekraus) requested a review from Dongbo Wang (daxian-dbw) October 17, 2017 22:23

Ilya (iSazonov) reviewed Oct 18, 2017

View reviewed changes

Ilya (iSazonov) self-assigned this Oct 18, 2017

Fix PowerShell#5139

8500b41

Ilya (iSazonov) reviewed Oct 18, 2017

View reviewed changes

Fix PowerShell#5139

24a1b22

Ilya (iSazonov) reviewed Oct 18, 2017

View reviewed changes

Fix PowerShell#5139

953c20b

Ilya (iSazonov) approved these changes Oct 18, 2017

View reviewed changes

Mark Kraus (markekraus) approved these changes Oct 18, 2017

View reviewed changes

Merge branch 'master' into master

13ec882

Mark Kraus (markekraus) approved these changes Oct 23, 2017

View reviewed changes

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

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

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026

Export-Csv Test Improvements (PowerShell#5150)

ac04c53

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

Conversation

Roman Marusyk (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!

Roman Marusyk (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!

Roman Marusyk (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!

Mark Kraus (markekraus) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Oct 23, 2017

Uh oh!

Roman Marusyk (Marusyk) commented Oct 23, 2017

Uh oh!

Mark Kraus (markekraus) commented Oct 23, 2017

Uh oh!

Mark Kraus (markekraus) commented Oct 23, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Oct 23, 2017

Uh oh!

Mark Kraus (markekraus) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Oct 24, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Oct 25, 2017

Uh oh!

Ilya (iSazonov) commented Oct 26, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Roman Marusyk (Marusyk) commented Oct 17, 2017 •

edited by iSazonov

Loading

Roman Marusyk (Marusyk) Oct 18, 2017 •

edited

Loading

Roman Marusyk (Marusyk) Oct 18, 2017 •

edited

Loading