Skip to content

Make ConvertTo-Html work on CoreCLR #2241

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
mirichmo merged 2 commits into from
Sep 22, 2016
Merged

Make ConvertTo-Html work on CoreCLR #2241

mirichmo merged 2 commits into from
Sep 22, 2016

Conversation

@jeffbi
Copy link
Contributor

@jeffbi jeffbi commented Sep 12, 2016

Part of issue #2240

This also includes some Pester test for the cmdlet.

Make ConvertTo-Html work on CoreCLR
e8481e6 
This also includes some Pester test for the cmdlet.
@msftclas
Copy link

msftclas commented Sep 12, 2016

Hi @jeffbi, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

vors
vors reviewed Sep 12, 2016
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Html.Tests.ps1 Outdated
@@ -0,0 +1,111 @@
Describe "ConvertTo-Html DRT Unit Tests" -Tags "CI" {
Copy link
Collaborator

@vors vors Sep 12, 2016

Choose a reason for hiding this comment

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

We are trying to move away from the Windows BVT/DRT terminology.
Please, just use the descriptive names for the Describe. More testing guidelines are here: https://github.com/PowerShell/PowerShell/blob/master/docs/testing-guidelines/WritingPesterTests.md#writing-pester-tests

@vors vors self-assigned this Sep 12, 2016
@vors
Copy link
Collaborator

vors commented Sep 12, 2016

Hi @jeffbi ! Thank you for your contribution.

Based on CI logs from appveyor, looks like you hit the problem that was discussed here #2182 (comment)

Multi-line strings are not very reliable for testing, because they depend on OS and the way how test file itself was checkout (for instance on AppVeyor all line ends for git files are \n, not \r\n).

In general, it's better to avoid using multi-line lines in the tests.
This case seems like an exception, where it's reasonable.
You can add code to strip-out line endings, similar to this:

https://github.com/PowerShell/platyPS/blob/32cc4b625348e40ce39dd12da352eabc203720f9/test/Pester/PlatyPs.Tests.ps1#L612

[-] Test ConvertTo-Html with no parameters 100ms
   Expected string length 386 but was 396. Strings differ at index 110.
   Expected: {<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"  "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">\n<html xmlns="http://www.w3.org/1999/xhtml">\n<head>\n<title>HTML TABLE</title>\n</head><body>\n<table>\n<colgroup><col/><col/><col/></colgroup>\n<tr><th>Name</th><th>Age</th><th>Friends</th></tr>\n<tr><td>John Doe</td><td>42</td><td>System.Object[]</td></tr>\n</table>\n</body></html>}
   But was:  {<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"  "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">\r\n<html xmlns="http://www.w3.org/1999/xhtml">\r\n<head>\r\n<title>HTML TABLE</title>\r\n</head><body>\r\n<table>\r\n<colgroup><col/><col/><col/></colgroup>\r\n<tr><th>Name</th><th>Age</th><th>Friends</th></tr>\r\n<tr><td>John Doe</td><td>42</td><td>System.Object[]</td></tr>\r\n</table>\r\n</body></html>}

mirichmo
mirichmo reviewed Sep 13, 2016
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Html.Tests.ps1 Outdated
@@ -0,0 +1,111 @@
Describe "ConvertTo-Html DRT Unit Tests" -Tags "CI" {
$customObject = [pscustomobject]@{"Name" = "John Doe"; "Age" = 42; "Friends" = ("Jack", "Jill")}
$newLine = [System.Environment]::NewLine
Copy link
Member

@mirichmo mirichmo Sep 13, 2016

Choose a reason for hiding this comment

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

This test code should go within BeforeAll instead of existing within the Describe. See Pester Do's and Don'ts

Copy link
Contributor Author

@jeffbi jeffbi Sep 14, 2016

Choose a reason for hiding this comment

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

Done. New commit pushed up.

From: Mike Richmond [mailto:notifications@github.com]
Sent: Tuesday, September 13, 2016 1:51 PM
To: PowerShell/PowerShell PowerShell@noreply.github.com
Cc: Jeff Bienstadt JeffBi@jetstreamsoftware.com; Mention mention@noreply.github.com
Subject: Re: [PowerShell/PowerShell] Make ConvertTo-Html work on CoreCLR (#2241)

In test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Html.Tests.ps1#2241 (comment):

@@ -0,0 +1,111 @@

+Describe "ConvertTo-Html DRT Unit Tests" -Tags "CI" {

  • $customObject = [pscustomobject]@{"Name" = "John Doe"; "Age" = 42; "Friends" = ("Jack", "Jill")}
  • $newLine = [System.Environment]::NewLine

This test code should go within BeforeAll instead of existing within the Describe. See Pester Do's and Don'tshttps://github.com/PowerShell/PowerShell/blob/master/docs/testing-guidelines/PesterDoAndDont.md

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/2241/files/e8481e6a53c1d6d2e84c09dc22c2fa94fa8dcf67#r78641559, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AGZehET8cnXNuO1cSEqZ2TNnZ-g7Ffa6ks5qpwy7gaJpZM4J7Etq.

@msftclas
Copy link

msftclas commented Sep 14, 2016

@jeffbi, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@mirichmo
Copy link
Member

mirichmo commented Sep 14, 2016

LGTM.

I'll merge it once we get resolution on the CLA issue.

@mirichmo mirichmo assigned mirichmo and unassigned vors Sep 14, 2016
@jeffbi
Copy link
Contributor Author

jeffbi commented Sep 14, 2016 •
edited
Loading

The CLA has been signed. Is there still an issue with it?

mirichmo
mirichmo suggested changes Sep 15, 2016
View reviewed changes
Copy link
Member

@mirichmo mirichmo left a comment

Choose a reason for hiding this comment

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

Blocking this PR pending resolution of the CLA issue

@mirichmo mirichmo closed this Sep 21, 2016
@mirichmo mirichmo reopened this Sep 21, 2016
@msftclas
Copy link

msftclas commented Sep 21, 2016

Hi @jeffbi, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@mirichmo mirichmo merged commit 5c88dbf into PowerShell:master Sep 22, 2016
@jeffbi jeffbi deleted the convertto-html-coreclr branch September 22, 2016 22:53
@mirichmo mirichmo mentioned this pull request Jul 17, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mirichmo mirichmo mirichmo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mirichmo mirichmo

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jeffbi @msftclas @vors @mirichmo @daxian-dbw @TravisEz13