Conversation
|
Tests failure due to UTF-8 BOM in |
|
@iSazonov I took measurements from e754367 took 48 seconds there is over 100x overhead to run seperate tests... |
Perster |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
@xtqqczze Please continue the PR. |
|
@iSazonov what is your opinion on moving |
|
rebased to clean up commit history |
Intention was to put in the folder all common tests, not only formatting/markdown. |
I'm not quite sure what is "common" about such tests since these tests do not run in each pipeline, only in |
|
I hope @TravisEz13 makes a conclusion about "move test\common to test\formatting". |
|
The test should be under common. We shouldn't create a new structure. If a formatting folder is needed, make it under common. |
|
@TravisEz13 push forced to remove change to folder structure |
There was a problem hiding this comment.
There are some BOM kinds. I'd expect we check all.
There was a problem hiding this comment.
There is only one kind of BOM for UTF-8. Perhaps in new PR we could verify all Unicode files are in UTF-8, not UTF-16, UTF-32, etc.
There was a problem hiding this comment.
I don't understand why does not check the 3 bytes for BOMs in one place.
There was a problem hiding this comment.
The test should be "No BOM" and check a preambula for all cases.
https://www.powershellgallery.com/packages/PSTemplatizer/1.0.20/Content/Functions%5CGet-FileEncoding.ps1
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
@xtqqczze Please rebase to get latest CI updates. |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
| It 'No UTF-8 BOM' { | ||
| filter hasUtf8Bom { | ||
| $_.Where{ | ||
| $bom = [Text.UnicodeEncoding]::UTF8.GetPreamble() |
There was a problem hiding this comment.
I believe it is better to detect the encoding. C# code:
Encoding GetPathEncoding()
{
using (StreamReader reader = new StreamReader(this.Path, Utils.utf8NoBom, detectEncodingFromByteOrderMarks: true))
{
_ = reader.Read();
return reader.CurrentEncoding;
}
}| @@ -0,0 +1,30 @@ | |||
| # Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
We should actually run this test somewhere
There was a problem hiding this comment.
Perhaps, add another job to here
https://github.com/PowerShell/PowerShell/blob/master/.vsts-ci/misc-analysis.yml
Or add another workflow:
Whichever you are more comfortable with
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment. |
PR Summary
test\commontotest\formattingPR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.