fix: restart dev-server on config change #21212

ZachJW34 · 2022-04-26T21:18:54Z

Closes UNIFY-1236

User facing changelog

Restart dev-server when config is refreshed

Additional details

The component dev-server is killed whenever the Cypress config changes. A new child process is created, but the dev-server requires an explicit start for it to actually start since it is a function that is registered on the dev-server:start event. This was only being called once during project initialization so any subsequent changes to the config would never restart the server.

This PR moves the devServer.start event into the config loading process such that whenever the config is refreshed, the dev-server will be recreated.

How has the user experience changed?

Screen.Recording.2022-04-26.at.10.23.53.AM.mov

PR Tasks

Have tests been added/updated?
Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?
Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2022-04-26T21:18:57Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-04-26T21:38:03Z

Test summary

4346 • 1 • 48 • 0 • 1

Run details


Project	cypress
Status	Failed
Commit	`d55e8b2`
Started	May 1, 2022 11:30 PM
Ended	May 1, 2022 11:44 PM
Duration	13:50 💡
OS	Linux Debian - 10.10
Browser	Electron 94

View run in Cypress Dashboard ➡️

Failures

	cypress/e2e/commands/files.cy.js		1
1	... > has implicit existence assertion, retries and throws a specific error when file does not exist for null encoding

Flakiness

	cypress/e2e/commands/net_stubbing.cy.ts		1
1	network stubbing > intercepting request > can delay and throttle a StaticResponse

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

…ver-reconnect

ZachJW34 · 2022-04-27T15:16:22Z

packages/data-context/src/data/ProjectLifecycleManager.ts

+        }
+
+        if (this._currentTestingType === 'component') {
+          const devServerOptions = await this.ctx._apis.projectApi.getDevServer().start({ specs: this.ctx.project.specs, config: finalConfig })


There is an unhandled edge-case here. If the user modifies their webpack config such that the port of the dev-server is changed, serving tests will fail. We would need to shut down and restart the server in order to fix this, but this seems like a fairly large refactor and not just ct specific.

I haven't done an audit of all config options, but there are a few options that would require a server restart such as watchForFileChanges or hosts which we currently aren't handling.

It would be CT specific since E2E does not use the dev-server.

Also isn't the port always randomly assigned each time webpack-dev-server starts?

So far my testing has shown that the dev-servers will default to the same port used, I only saw a port switch when explicitly changing the port in the webpackConfig. What I have wired up isn't fullproof, but we have UNIFY-1696 to properly handle config changes that require a server restart.

tgriesser · 2022-04-27T15:27:59Z

packages/app/cypress/e2e/cypress-in-cypress-component.cy.ts

-    cy.openProject('cypress-in-cypress')
-    cy.startAppServer('component')


Is there a reason we needed to move this from the beforeEach?

There was another test that were duplicating the openProject and startAppServer call with different arguments, I'm not a fan of calling these more than once so I decided to move them out and into each test.

Yeah that's fine, I wonder if that's an indication we should have a second describe block then?

Addressed in 8cabb60

tgriesser · 2022-04-27T15:34:04Z

packages/server/lib/project-base.ts

-    const { ctDevServerPort } = await this.initializeSpecsAndDevServer(cfg)
-
-    if (this.testingType === 'component') {
-      cfg.baseUrl = `http://localhost:${ctDevServerPort}`
-    }
-


Glad we're getting rid of this from here

tgriesser · 2022-04-27T15:37:34Z

packages/data-context/src/data/ProjectLifecycleManager.ts

+      if (this._currentTestingType === 'component') {
+        // Since we refresh the dev-server on config changes, we need to close it and clean up it's listeners
+        // before we can start a new one. This needs to happen before we have registered the events of the child process
+        this.ctx._apis.projectApi.getDevServer().close()


Looking at the implementation of getDevServer().close() it looks like it sends close, but I don't see anywhere that it's listened to on the child end 🤔 .

There's also this block:

baseEmitter.on('dev-server:close', () => { debug('base emitter plugin close event') ipc.send('dev-server:close') })

but I don't see anywhere that's in-use, either on the issuing end or the receiving end?

Was mostly looking for this close contract to see if we should be await'ing it being closed, rather than firing & trying to re-init, but if I'm reading the code correctly, it doesn't even seem like it's being used - I think we're just relying on it getting auto-closed when the child process is killed.

So my question is - do we want to clean this up so we're closing the server properly?

I'm honestly not too familiar with the IPC communication, I was just trying to lift and keep the same contract that existed before. There is a close function returned with the dev-servers, maybe this is the missing link. Like you said, seems like we are just relying on the child process being auto-killed, I can confirm that the new server spawned doesn't conflict with the old as there would be a port in use error thrown but maybe there is a potential race condition here. I'll dig in a bit, could use some help in this area though

I dug into this a bit and landed on just removing the unused close functions and event. Killing the child process is safe in terms of cleanup, and we weren't calling the close functions from the dev-servers to begin with. I think we could put in a system to allow these child processes to have a cleanup step, but that's outside of the scope of this issue. I can followup with an issue if this is something we should pursue.

ryanthemanuel · 2022-04-27T19:43:00Z

packages/app/cypress/e2e/cypress-in-cypress-component.cy.ts

+      sinon.stub(ctx._apis.projectApi.getDevServer(), 'close')
+      const devServerReady =
+        new Promise((res) => {
+          ctx._apis.projectApi.getDevServer().emitter.on('dev-server:compile:success', (err) => res(true))


Are we doing anything with the value that's resolved? Should this be:

Suggested change

ctx._apis.projectApi.getDevServer().emitter.on('dev-server:compile:success', (err) => res(true))

ctx._apis.projectApi.getDevServer().emitter.on('dev-server:compile:success', (err) => res())

Addressed in 8cabb60

ryanthemanuel · 2022-04-27T19:43:41Z

packages/app/cypress/e2e/specs_list_actual_git_repo.cy.ts

@@ -2,10 +2,10 @@ describe('Spec List - Git Status', () => {
  beforeEach(() => {
    cy.scaffoldProject('cypress-in-cypress')
    .then((projectPath) => {
-      cy.task('initGitRepoForTestProject', projectPath)
      cy.wait(500)


If we're moving stuff around, is this wait still needed?

Nope! 4bd8b90

lmiller1990

Definitely fixes a lot of bugs, but looks like we need a bit of work around the hot reload - seems HMR dies after the restart.

I commented in the issue and added a video reproduction: https://cypress-io.atlassian.net/browse/UNIFY-1236?focusedCommentId=17992

lmiller1990 · 2022-04-29T01:22:05Z

packages/data-context/src/data/ProjectLifecycleManager.ts

+        }
+
+        if (this._currentTestingType === 'component') {
+          const devServerOptions = await this.ctx._apis.projectApi.getDevServer().start({ specs: this.ctx.project.specs, config: finalConfig })


It would be CT specific since E2E does not use the dev-server.

Also isn't the port always randomly assigned each time webpack-dev-server starts?

packages/data-context/src/data/ProjectLifecycleManager.ts

lmiller1990 · 2022-04-29T01:26:00Z

packages/server/lib/project-base.ts

@@ -308,46 +300,6 @@ export class ProjectBase<TServer extends Server> extends EE {
    options.onError(err)
  }

-  async initializeSpecsAndDevServer (updatedConfig: Cfg): Promise<{


Nice, I love deleting code from project-base - I think we should be mostly getting rid of this entire thing, eventually.

ZachJW34 · 2022-04-29T22:41:49Z

@lmiller1990 I'll create a followup issue for vite not working. Since webpack is our primary audience, I'm ok with getting this merged. I believe UNIFY-1696 will fix the vite issue.

lmiller1990

Known bug with the Vite restart, issue is logged.

fix: restart dev-server on config change

adb544e

ZachJW34 and others added 4 commits April 26, 2022 17:29

close dev server before cp is spawned

6ca76ac

Merge remote-tracking branch 'origin/10.0-release' into zachw/dev-ser…

12eeaec

…ver-reconnect

fix test that is failing... not sure why

daecde8

Merge branch '10.0-release' into zachw/dev-server-reconnect

fa859c2

ZachJW34 commented Apr 27, 2022

View reviewed changes

ZachJW34 requested review from ryanthemanuel and marktnoonan April 27, 2022 15:18

ZachJW34 marked this pull request as ready for review April 27, 2022 15:18

ZachJW34 requested review from a team and tgriesser as code owners April 27, 2022 15:18

tgriesser requested changes Apr 27, 2022

View reviewed changes

ryanthemanuel reviewed Apr 27, 2022

View reviewed changes

ZachJW34 and others added 3 commits April 28, 2022 18:17

cleanup unsued close events

8cabb60

remove wait

4bd8b90

Merge branch '10.0-release' into zachw/dev-server-reconnect

00c647b

ZachJW34 requested a review from tgriesser April 28, 2022 23:23

lmiller1990 reviewed Apr 29, 2022

View reviewed changes

MuazOthman requested review from ryanthemanuel and lmiller1990 April 29, 2022 15:37

add back in close for unit tests

db0ecad

Merge branch '10.0-release' into zachw/dev-server-reconnect

d55e8b2

tgriesser approved these changes Apr 30, 2022

View reviewed changes

lmiller1990 approved these changes May 2, 2022

View reviewed changes

lmiller1990 merged commit 00a0f5a into 10.0-release May 2, 2022

lmiller1990 deleted the zachw/dev-server-reconnect branch May 2, 2022 02:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: restart dev-server on config change #21212

fix: restart dev-server on config change #21212

ZachJW34 commented Apr 26, 2022

cypress-bot bot commented Apr 26, 2022

cypress bot commented Apr 26, 2022 •

edited

Loading

ZachJW34 Apr 27, 2022

lmiller1990 Apr 29, 2022

ZachJW34 Apr 29, 2022

tgriesser Apr 27, 2022

ZachJW34 Apr 27, 2022

tgriesser Apr 27, 2022

ZachJW34 Apr 28, 2022

tgriesser Apr 27, 2022

tgriesser Apr 27, 2022

ZachJW34 Apr 27, 2022

ZachJW34 Apr 28, 2022

ryanthemanuel Apr 27, 2022

ZachJW34 Apr 28, 2022

ryanthemanuel Apr 27, 2022

ZachJW34 Apr 28, 2022

lmiller1990 left a comment

lmiller1990 Apr 29, 2022

lmiller1990 Apr 29, 2022

ZachJW34 commented Apr 29, 2022

lmiller1990 left a comment

		cy.openProject('cypress-in-cypress')
		cy.startAppServer('component')

	ctx._apis.projectApi.getDevServer().emitter.on('dev-server:compile:success', (err) => res(true))
	ctx._apis.projectApi.getDevServer().emitter.on('dev-server:compile:success', (err) => res())

fix: restart dev-server on config change #21212

fix: restart dev-server on config change #21212

Conversation

ZachJW34 commented Apr 26, 2022

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Apr 26, 2022

cypress bot commented Apr 26, 2022 • edited Loading

Test summary

Run details

Failures

Flakiness

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZachJW34 commented Apr 29, 2022

lmiller1990 left a comment

Choose a reason for hiding this comment

cypress bot commented Apr 26, 2022 •

edited

Loading