Fix ConvertTo-Html single matched column header bug #2184
Conversation
Fix ConvertTo-Html single matched column header bug Add ConvertTo-Html pester tests for single matched column header bug
|
Hi @TimCurwick, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
|
Fixes issue #2185 |
|
|
||
| [Cmdlet(VerbsData.ConvertTo, "Html", DefaultParameterSetName = "Page", | ||
| HelpUri = "https://go.microsoft.com/fwlink/?LinkID=113290", RemotingCapability = RemotingCapability.None)] | ||
| HelpUri = "http://go.microsoft.com/fwlink/?LinkID=113290", RemotingCapability = RemotingCapability.None)] |
There was a problem hiding this comment.
Could you revert this to https please. TLS connections should be the default for URLs.
|
@adityapatwardhan Please take a look. This fix is specific to Windows PowerShell (full CLR) because the cmdlet has not yet been ported to CoreCLR. |
| $customPSObject = [pscustomobject]@{ "prop1" = "val1"; "prop2" = "val2" } | ||
|
|
||
| It "Test ConvertTo-Html header with default single property" -Skip:( $IsLinux -or $IsOSX -or $IsCoreCLR ) { | ||
| $returnObject = $customPSObject | Select prop1 | ConvertTo-Html |
There was a problem hiding this comment.
Select [](start = 42, length = 6)
Do not use aliases in tests. Use select-object instead.
|
@TimCurwick please fix the two small comments in the tests. Rest looks good. |
|
@TimCurwick could you please address the 2 comments and rebase your commits? |
|
Sorry about the delay. I was on vacation and then busy with the Summit. Airports are great for catching up on work. :) |
|
@TimCurwick Seems that there are merge conflicts in test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Html.Tests.ps1. Could you resolve those please? |
|
@TimCurwick Commit# 6b1ecd4 also has merge conflicts. |
|
please resolve the build conflict. It's still there |
|
I am trying to clean out old PRs and this one looks potentially useful. @JamesWTruher or @dantraMSFT - Can one or both of you review this change and provide feedback? Also, please confirm that it is still relevant and has not already been fixed. If you guys approve of it, I'll guide it in for a landing. |
|
|
||
| function normalizeLineEnds([string]$text) | ||
| { | ||
| $text -replace "`r`n?|`n", "`r`n" |
There was a problem hiding this comment.
$text -replace "${newLine}?|`n","${newLine}"
might be better, or could the line endings be removed entirely? Is that part of what you're looking to validate?
| <title>HTML TABLE</title> | ||
| </head><body> | ||
| <table> | ||
| <colgroup><col/><col/><col/></colgroup> |
There was a problem hiding this comment.
i sure wish we could replicate less code - create variables and reuse them?
|
|
||
| It "Test ConvertTo-Html with no parameters" { | ||
| $returnObject = $customObject | ConvertTo-Html | ||
| $returnObject -is [System.Array] | Should Be $true |
There was a problem hiding this comment.
this test isn't really needed, right? isn't the test on line 30 enough?
There was a problem hiding this comment.
I think the test is appropriate; however, it would be easier to diagnose if Type.FullName is compared instead of a soft-cast.
There was a problem hiding this comment.
Should Be $true is an anti-pattern - the logs aren't very useful so try to avoid that.
In this case, you can use 'Should BeOfType`. See the list of Pester assertions here.
And I'll be honest - I'm surprised it's an array - I would have expected a single string, so from that point of view, it might be worth a test to at least make it obvious if someone were to implement my expected behavior.
|
@TimCurwick are you planning to implemented the requested changes? |
|
I replaced this PR with PR #4276 |
13 participants
Fix ConvertTo-Html single matched column header bug
Add ConvertTo-Html pester tests for single matched column header bug