Skip to content

Add Unicode formatting tests #12639

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
Closed
xtqqczze wants to merge 2 commits into from
Closed

Add Unicode formatting tests #12639

xtqqczze wants to merge 2 commits into from

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented May 13, 2020 •
edited
Loading

PR Summary

  • exclude UTF-8 BOM from text files
  • move test\common to test\formatting

PR Context

PR Checklist

@ghost ghost assigned anmenaga May 13, 2020
@xtqqczze
Copy link
Contributor Author

xtqqczze commented May 13, 2020

Tests failure due to UTF-8 BOM in ./test/common/utf8/utf8.tests.ps1, will push a commit to remove after review.

iSazonov
iSazonov reviewed May 13, 2020
View reviewed changes
@xtqqczze
Copy link
Contributor Author

xtqqczze commented May 14, 2020 •
edited
Loading

@iSazonov I took measurements from PowerShell-CI-static-analysis on how execution time for utf8.tests.ps1

e754367 took 48 seconds
3c864ac took 350 milliseconds

there is over 100x overhead to run seperate tests...

@iSazonov
Copy link
Collaborator

iSazonov commented May 14, 2020 •
edited
Loading

there is over 100x overhead to run seperate tests...

Perster Context and It blocks slow down execution. I think we could report only errors.
Maybe we could use Set-ItResult.

@ghost ghost added the Review - Needed The PR is being reviewed label May 27, 2020
@ghost
Copy link

ghost commented May 27, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

@iSazonov
Copy link
Collaborator

iSazonov commented May 27, 2020

@xtqqczze Please continue the PR.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented May 27, 2020 •
edited
Loading

@iSazonov what is your opinion on moving test/common to test/formatting

@xtqqczze xtqqczze changed the title WIP: Create utf8.tests.ps1 Add Unicode formatting tests May 27, 2020
@xtqqczze
Copy link
Contributor Author

xtqqczze commented May 27, 2020

rebased to clean up commit history

@iSazonov
Copy link
Collaborator

iSazonov commented May 27, 2020

what is your opinion on moving test/common to test/formatting

Intention was to put in the folder all common tests, not only formatting/markdown.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented May 27, 2020

what is your opinion on moving test/common to test/formatting

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 PowerShell-CI-static-analysis.

@TravisEz13 TravisEz13 added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed Review - Waiting on Author labels May 27, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented May 28, 2020

I hope @TravisEz13 makes a conclusion about "move test\common to test\formatting".

@TravisEz13
Copy link
Member

TravisEz13 commented May 30, 2020

The test should be under common. We shouldn't create a new structure. If a formatting folder is needed, make it under common.

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 30, 2020
@xtqqczze
Copy link
Contributor Author

xtqqczze commented May 30, 2020

@TravisEz13 push forced to remove change to folder structure

iSazonov
iSazonov reviewed May 31, 2020
View reviewed changes
test/common/FileEncoding/FileEncoding.Tests.ps1 Outdated
Copy link
Collaborator

@iSazonov iSazonov May 31, 2020

Choose a reason for hiding this comment

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

There are some BOM kinds. I'd expect we check all.

Copy link
Contributor Author

@xtqqczze xtqqczze May 31, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@iSazonov iSazonov May 31, 2020

Choose a reason for hiding this comment

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

I don't understand why does not check the 3 bytes for BOMs in one place.

Copy link
Collaborator

@iSazonov iSazonov Jun 1, 2020

Choose a reason for hiding this comment

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

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

@ghost ghost added the Review - Needed The PR is being reviewed label Jun 8, 2020
@ghost
Copy link

ghost commented Jun 8, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

@iSazonov
Copy link
Collaborator

iSazonov commented Nov 5, 2020

@xtqqczze Please rebase to get latest CI updates.

@ghost ghost removed the Review - Needed The PR is being reviewed label Nov 5, 2020
@ghost ghost added the Review - Needed The PR is being reviewed label Nov 12, 2020
@ghost
Copy link

ghost commented Nov 12, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

iSazonov
iSazonov reviewed Nov 13, 2020
View reviewed changes
test/common/FileEncoding/FileEncoding.Tests.ps1
It 'No UTF-8 BOM' {
filter hasUtf8Bom {
$_.Where{
$bom = [Text.UnicodeEncoding]::UTF8.GetPreamble()
Copy link
Collaborator

@iSazonov iSazonov Nov 13, 2020

Choose a reason for hiding this comment

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

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;
                }
            }

@daxian-dbw daxian-dbw added the CommunityDay-Small A small PR that the PS team has identified to prioritize to review label May 15, 2023
TravisEz13
TravisEz13 requested changes May 22, 2023
View reviewed changes
test/common/FileEncoding/FileEncoding.Tests.ps1
@@ -0,0 +1,30 @@
# Copyright (c) Microsoft Corporation.
Copy link
Member

@TravisEz13 TravisEz13 May 22, 2023

Choose a reason for hiding this comment

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

We should actually run this test somewhere

Copy link
Member

@TravisEz13 TravisEz13 May 22, 2023

Choose a reason for hiding this comment

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

Perhaps, add another job to here
https://github.com/PowerShell/PowerShell/blob/master/.vsts-ci/misc-analysis.yml

Or add another workflow:

workflows

Whichever you are more comfortable with

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels May 22, 2023
@ghost ghost added the Stale label Jun 6, 2023
@ghost
Copy link

ghost commented Jun 6, 2023

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.

@ghost ghost closed this Jun 17, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 TravisEz13 TravisEz13 requested changes

@iSazonov iSazonov iSazonov left review comments

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

Assignees

@anmenaga anmenaga

Labels

CommunityDay-Small A small PR that the PS team has identified to prioritize to review Stale Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Write test to exclude UTF-8 BOM from files in tree

5 participants

@xtqqczze @iSazonov @TravisEz13 @daxian-dbw @anmenaga