Skip to content
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

Fix htmlnano tests #2532

Closed
wants to merge 3 commits into from
Closed

Fix htmlnano tests #2532

wants to merge 3 commits into from

Conversation

mischnic
Copy link
Member

↪️ Pull Request

The version upgrade in #2506 seems to have changed the output regarding collapseWhitespace. Setting that htmlnano option to all would make current tests pass, but is not always safe (see here)

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

DeMoorJasper
DeMoorJasper requested changes Jan 12, 2019
View reviewed changes
Copy link
Member

@DeMoorJasper DeMoorJasper left a comment •
edited

Choose a reason for hiding this comment

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

Appears this doesn't fix the tests?

@mischnic
Copy link
Member Author

Appears this doesn't fix the tests?

Strange, it does locally.
Only some commits on master after the mentioned PR fail due to the additional newline, but not all.
Also, that PR shouldn't have changed anything regarding testing, the lockfile already contained that version. 🤔

@devongovett
Copy link
Member

Yeah seems like it's still failing on windows

@mischnic
Copy link
Member Author

mischnic commented Jan 17, 2019 •
edited

Yeah seems like it's still failing on windows

Strange, no newline on windows:

<!doctype html><html><head><title>Test</title></head><body> <h1>Other page</h1> </body></html>

@mischnic mischnic mentioned this pull request Jan 20, 2019
Merged
3 tasks
@DeMoorJasper DeMoorJasper mentioned this pull request Jan 28, 2019
Closed
@DanielRuf
Copy link

Windows uses \r\n afaik.

@millette
Copy link

Maybe use https://nodejs.org/dist/latest-v10.x/docs/api/os.html#os_os_eol in that case?

@DanielRuf
Copy link

Maybe use nodejs.org/dist/latest-v10.x/docs/api/os.html#os_os_eol in that case?

Definitely better and something what I would recommend. But this could fail in some cases but let's try it.

@mischnic
Copy link
Member Author

Definitely better and something what I would recommend. But this could fail in some cases but let's try it.

If you're running Windows, please try that out. But if I remember correctly, the output contained no linebreak at all, it really was the output in #2532 (comment).

@DanielRuf
Copy link

Will do it probably later this week, currently checking some projects and working on a few other CI fixes.

@DeMoorJasper DeMoorJasper mentioned this pull request Jan 29, 2019
Merged
3 tasks
@mischnic mischnic deleted the htmlnano-test branch February 25, 2019 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DeMoorJasper DeMoorJasper DeMoorJasper requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mischnic @devongovett @DanielRuf @millette @DeMoorJasper