Skip to content

fix: assets ?url import test for windows, fix #2625#2631

Merged
patak-cat merged 3 commits intovitejs:mainfrom
patak-cat:fix/test-assets-url-import
Mar 29, 2021
Merged

fix: assets ?url import test for windows, fix #2625#2631
patak-cat merged 3 commits intovitejs:mainfrom
patak-cat:fix/test-assets-url-import

Conversation

@patak-cat
Copy link
Copy Markdown
Member

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Description

Check #2625, looks like in windows the line end in foo.js was \r\n, and in the tests the line end was \n. This test then failed only in Windows.

I initially thought of removing the line ends since they are not needed to test the ?url feature, but the that is not possible because of prettier. This PR fixes the test in windows by matching with a startsWidth regex instead. I think it is enough for testing this feature.

@patak-cat patak-cat added the p1-chore Doesn't change code behavior (priority) label Mar 21, 2021
Shinigami92
Shinigami92 previously approved these changes Mar 22, 2021
@Shinigami92
Copy link
Copy Markdown
Member

Shinigami92 commented Mar 22, 2021

Are you sure that this fixes the test?
Both cases seem to differ in ending.

- data:application/javascript;base64,Y29uc29sZS5sb2coJ2hpJykK
+ data:application/javascript;base64,Y29uc29sZS5sb2coJ2hpJykNCg==
#                                                           ^^^^^

@patak-cat
Copy link
Copy Markdown
Member Author

@Shinigami92 it was working, but you are right. There is also padding involved.
We discussed with @GrygrFlzr in ViteLand, just changed it to read the file from disk and compare as before.

Added a readFile utility, following the naming of editFile and other utilities in test utils

@patak-cat patak-cat requested a review from Shinigami92 March 22, 2021 07:02
@Shinigami92
Copy link
Copy Markdown
Member

I think we can use readFile also in line 68+69, can we?

@patak-cat
Copy link
Copy Markdown
Member Author

We can't because we need the resolved filename for writeFileSync in line 71. We could create a writeFile util, but maybe better to go there when it is needed.

@Shinigami92
Copy link
Copy Markdown
Member

Ah I see, okay 🙂

@Shinigami92 Shinigami92 requested a review from GrygrFlzr March 22, 2021 08:14
@patak-cat patak-cat requested a review from antfu March 22, 2021 09:14
@patak-cat patak-cat linked an issue Mar 22, 2021 that may be closed by this pull request
3 tasks
@Shinigami92 Shinigami92 mentioned this pull request Mar 23, 2021
9 tasks
@patak-cat patak-cat merged commit 6743e07 into vitejs:main Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p1-chore Doesn't change code behavior (priority) test windows only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test-build not successful on windows

3 participants