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.

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

fix: server.address before listen, chdir in test, basic cli test #5059

Merged
merged 17 commits into from Sep 27, 2021

Conversation

dominikg
Copy link
Contributor

Description

today vite-2.6.0-beta.1 release broke cli on node < 16 and it went unnoticed because the current tests never use the cli

This PR adds a new cli playground that uses a custom serve.js implementation that runs vite, vite build and vite preview in a subprocess.

The tests themsselves are very basic, just checking that the page comes up with expected content. No tests for arguments or anything.

Additional context

killing of the subprocess is a pain, in vite-plugin-svelte i used tree-kill for that because there's another indirection. Here it relies on execa's kill.
There are relatively short timeouts on process handling to avoid running out of the 10s jest limit, hopefully this works on CI too, otherwise it might need a larger timeout.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

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.

@dominikg
Copy link
Contributor Author

not cool windows, not cool... looks like e.killed isn't true when the process was actually killed :( might take me until tomorrow to get to the bottom of this.

If anyone has ideas how to actually kill a process on windows, please chime in.

@dominikg
Copy link
Contributor Author

this is going to have to wait until #5060 has been merged as it's likely this needs to be adapted to be usable with pnpm.

@dominikg
Copy link
Contributor Author

force-pushed to update to pnpm

@patak-dev
Copy link
Member

I tried and failed to kill the process in windows. Maybe we should avoid this test in windows until we figure this out? At least we can check that the cli is working correctly in other environments (so missing imports at least will not pass).

@dominikg
Copy link
Contributor Author

on windows it only works with taskkill see https://github.com/pkrumins/node-tree-kill/blob/cb478381547107f5c53362668533f634beff7e6e/index.js#L29

i'll add that later tonight. might also look into group killing for unix to be extra safe.

@dominikg
Copy link
Contributor Author

dominikg commented Sep 26, 2021

i'm at a loss here, the tests run fine locally, but fail on github ci with a strange error that the server isn't starting
https://github.com/vitejs/vite/runs/3710496041?check_suite_focus=true#step:9:87

stdout:

        vite v2.6.0-beta.2 dev server running at:

stderr:

      error when starting dev server:
      TypeError: Cannot read property 'address' of null
          at isAddressInfo (/home/runner/work/vite/vite/packages/vite/src/node/logger.ts:149:57)
          at Object.printHttpServerUrls (/home/runner/work/vite/vite/packages/vite/src/node/logger.ts:150:7)
          at CAC.<anonymous> (/home/runner/work/vite/vite/packages/vite/src/node/cli.ts:109:7)

patak-dev
patak-dev previously approved these changes Sep 26, 2021
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

🚀🥳

Thanks for not giving up 👏

@dominikg dominikg changed the title test(cli): add playground with tests for cli fix(cli): correctly log server information on devserver start ; add basic cli testsuite for "vite", "vite build" and "vite preview" Sep 26, 2021
@dominikg dominikg changed the title fix(cli): correctly log server information on devserver start ; add basic cli testsuite for "vite", "vite build" and "vite preview" test(cli): add basic cli testsuite for "vite", "vite build" and "vite preview" Sep 26, 2021
@dominikg
Copy link
Contributor Author

dominikg commented Sep 26, 2021

this is now ready for review.

originally i only wanted to add the testsuite itself, but it uncovered 2 problems along the way, both of which are fixed here too.

  1. the testsuite for postcss-caching uses process.chdir which caused all tests after it to run from a different directory. It's a miracle that nothing else broke due to this.
  2. refactor: change location of server start log messages #5016 changed the logging of server information but tried to access server.address() before server.listen() was called and it changed the log output of startup time.

@patak-dev
Copy link
Member

Merging this one to be able to do a beta.3 release

@patak-dev patak-dev changed the title test(cli): add basic cli testsuite for "vite", "vite build" and "vite preview" fix: server.address before listen, chdir in test, basic cli test Sep 27, 2021
@patak-dev patak-dev merged commit fb37a63 into vitejs:main Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants