fix: subscribe to changes in cypress.config.js in app #21160

lmiller1990 · 2022-04-21T06:09:17Z

Closes https://cypress-io.atlassian.net/browse/UNIFY-1163

User facing changelog

Use subscriptions to update/respond correctly in app when cypress.config.js is updated. Now, if you update cypress.config.js on the runner page (eg change viewportHeight) it will re-execute the spec with the new config.

It can feel a bit unresponsive, since we re-run the child process to get the latest config. We will add an overlay with "Loading..." to give the user immediate feedback that we are reloading, that'll be part of another PR, see this comment.

Additional details

I also found a bug where CT fails to reconnect to the dev server after you change cypress.config.js. https://cypress-io.atlassian.net/browse/UNIFY-1675. It's outside the scope of this ticket, though, but we should look into this as a fast follow up.

How has the user experience changed?

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-21T06:09:20Z

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-21T08:24:51Z

Test summary

17881 • 0 • 217 • 0 • 2

Run details


Project	cypress
Status	Passed
Commit	`d396acc`
Started	Apr 22, 2022 6:37 AM
Ended	Apr 22, 2022 6:51 AM
Duration	14:11 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	xhr.cy.js	1
1	... > no status when request isnt forced 404
	actions/selectFile.cy.js	1
1	... > throws with aliased intercepts

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

tgriesser

I think we are actually going to also want to show the loading spinner when the background process is refreshing, @marktnoonan was looking into that as a separate ticket

tgriesser · 2022-04-21T13:26:18Z

packages/app/src/pages/Specs/Runner.vue

+  // if the `config` changed, we want to reload the entire
+  // page and re-execute the current test with the latest config
+  // values
+  // subscriptions trigger on the initial page load,
+  // so we do not want to trigger `window.location.reload` on the
+  // first load, or we get stuck in an infinite loop.


What if we send the config values in the subscription as a new field with a JSON payload, and then set them on window.__CYPRESS_CONFIG__?

I'll give this a try and see how it goes.

Your idea is quite a bit better in terms of reliability. I found something troubling; the config we source in data-context was missing some dynamic values like the proxyUrl. See those here: https://github.com/cypress-io/cypress/pull/21160/files#diff-06b7fcb589558cace004423a0085796fa17cfb870f200c97b66105c7400c69d6R45-R56

For now I patched it in, but we need to continue to aggressively triage the config problem we've got and find a single way to manage and source it.

I'd say the next step would be a solid refactor of project-base, open-project and server-{base,e2e,ct} and the surrounding area, but this is quite high risk, so not sure we should do that right now. I might start a tech brief where we can gather our thoughts about this.

marktnoonan · 2022-04-21T13:59:28Z

Yeah I was going to either write that up as a ticket or add it as a part of #21157

lmiller1990 · 2022-04-22T00:58:27Z

@tgriesser @marktnoonan I agree we should show a spinner here too, is there a mock for this? Should it be included in this ticket? It could be implemented separately.

lmiller1990 · 2022-04-22T05:46:05Z

packages/app/src/runner/index.ts

  // window.UnifiedRunner exists now, since the Webpack bundle with
  // the UnifiedRunner namespace was injected by `injectBundle`.
  initializeEventManager(window.UnifiedRunner)

  window.UnifiedRunner.MobX.runInAction(() => {
-    const store = initializeMobxStore(window.UnifiedRunner.config.testingType)


We can kill off most of window.UnifiedRunner soon

lmiller1990 · 2022-04-22T06:38:02Z

packages/app/src/pages/Specs/Runner.vue

+  }
+
+  if (!initialLoad && specStore.activeSpec) {
+    try {


Not super ideal, since we don't know that eventManager will absolute exist here (potential race condition, you changed cypress.config.js while the spec is still "Spec is loading...", so I just did a try/catch.

lmiller1990 · 2022-04-22T06:39:36Z

packages/data-context/src/sources/HtmlDataSource.ts

@@ -41,8 +42,32 @@ export class HtmlDataSource {
    throw err
  }

+  getUrlsFromLegacyProjectBase (cfg: any) {


The config we get in the data-context doesn't have some of these important keys. They are dynamically added later, for example the port will only be added once the CT dev server has started.

We'll need to find a way to better manage the config in general, but this fixes the problem for now and links the "legacy" part of config in project-base and server-{ct,e2e} with the newer data-context stuff.

marktnoonan · 2022-04-26T14:31:52Z

In testing I was able to reach one state surprised me a lot - no error shown and on refresh I saw this:

Turns out I saved, or the editor autosaved, with a viewportWidth of 00. I think the error handling in the App (PR #21157) will manage that situation, this error displays fine in the launchpad.

I also hit a broken state in CT but I think that makes sense based on the description here, I added a question: https://cypress-io.atlassian.net/browse/UNIFY-1236

Haven't finished reviewing the code yet, just wanted to share update.

marktnoonan · 2022-04-26T17:56:13Z

Found a potential issue when switching testing types, shown here: https://www.loom.com/share/79e5288cc9ea49bd8d88d454865e0dce

Not sure how to recreate it, just captured what I could.

lmiller1990 · 2022-04-26T19:20:39Z

Thanks for the update @marktnoonan, looks like so far it's okay except for that weird bug you found in loom - thanks for capturing that. I'm not sure if I can reproduce, but I'll give it a try.

If we can reproduce, it could be a separate PR? I can see this blowing out if we try to fix any and all config errors here - my main goal was to hook up subscriptions.

marktnoonan

All looks good and works well in e2e and on settings page, happy to create a follow up issue for my last comment, though since it is testing-type related, I will hold off until we have the config reloading not breaking CT, since that's a possible confounder.

marktnoonan · 2022-04-26T19:27:55Z

packages/app/src/runner/index.ts


    store.updateDimensions(config.viewportWidth, config.viewportHeight)
  })

-  window.UnifiedRunner.MobX.runInAction(() => setupRunner(config.namespace))


It looks like this function wasn't using config.namespace for anything, this is just cleanup?

Yep, unused.

ryanthemanuel · 2022-04-26T22:22:10Z

packages/data-context/src/actions/DataEmitterActions.ts

+   * either re-run a spec or update something in the App UI.
+   */
+  configChange () {
+    this._emit('configChange')


Do we envision that this can eventually negate the need for some of the toApp and toLaunchpad calls elsewhere?

Good question - for now I'm guessing we want to respond to any config changes in both. I can't think of any reason to selectively only update only app or only launchpad at this point.

ryanthemanuel

Looks good and works well. Nice work!

lmiller1990 added 3 commits April 20, 2022 17:01

chore: work on build

85807ed

subscribe to config changes

489c172

add test

e28c31e

extract required values from legacy config

4902564

tgriesser reviewed Apr 21, 2022

View reviewed changes

lmiller1990 added 4 commits April 22, 2022 13:54

add testingType and isComponentTesting to config

1826bc3

update cypress config

56799fb

update runner config on window and re-execute after config changes

ff8b8c8

revert change

f1fe243

lmiller1990 commented Apr 22, 2022

View reviewed changes

lmiller1990 added 2 commits April 22, 2022 15:47

revert code

39b7dfd

Merge branch '10.0-release' into lmiller1990/UNIFY-1163

c626a22

lmiller1990 force-pushed the lmiller1990/UNIFY-1163 branch from 0bb4e41 to c626a22 Compare April 22, 2022 05:50

lmiller1990 added 3 commits April 22, 2022 15:51

revert

8992347

revert file

8465c2e

update stub types

d396acc

lmiller1990 marked this pull request as ready for review April 22, 2022 06:36

lmiller1990 requested a review from a team as a code owner April 22, 2022 06:36

lmiller1990 requested review from jennifer-shehane, marktnoonan and a team and removed request for a team and jennifer-shehane April 22, 2022 06:36

lmiller1990 commented Apr 22, 2022

View reviewed changes

rockhold requested a review from ryanthemanuel April 26, 2022 15:38

marktnoonan approved these changes Apr 26, 2022

View reviewed changes

ryanthemanuel reviewed Apr 26, 2022

View reviewed changes

tgriesser approved these changes Apr 28, 2022

View reviewed changes

ryanthemanuel approved these changes Apr 28, 2022

View reviewed changes

lmiller1990 merged commit b65371a into 10.0-release Apr 29, 2022

lmiller1990 deleted the lmiller1990/UNIFY-1163 branch April 29, 2022 00:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: subscribe to changes in cypress.config.js in app #21160

fix: subscribe to changes in cypress.config.js in app #21160

lmiller1990 commented Apr 21, 2022 •

edited

Loading

cypress-bot bot commented Apr 21, 2022

cypress bot commented Apr 21, 2022 •

edited

Loading

tgriesser left a comment

tgriesser Apr 21, 2022

lmiller1990 Apr 22, 2022 •

edited

Loading

lmiller1990 Apr 22, 2022 •

edited

Loading

marktnoonan commented Apr 21, 2022

lmiller1990 commented Apr 22, 2022

lmiller1990 Apr 22, 2022

lmiller1990 Apr 22, 2022

lmiller1990 Apr 22, 2022

marktnoonan commented Apr 26, 2022

marktnoonan commented Apr 26, 2022

lmiller1990 commented Apr 26, 2022

marktnoonan left a comment

marktnoonan Apr 26, 2022

lmiller1990 Apr 26, 2022

ryanthemanuel Apr 26, 2022

lmiller1990 Apr 27, 2022

ryanthemanuel left a comment

fix: subscribe to changes in cypress.config.js in app #21160

fix: subscribe to changes in cypress.config.js in app #21160

Conversation

lmiller1990 commented Apr 21, 2022 • edited Loading

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Apr 21, 2022

cypress bot commented Apr 21, 2022 • edited Loading

Test summary

Run details

Flakiness

tgriesser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

lmiller1990 Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

marktnoonan commented Apr 21, 2022

lmiller1990 commented Apr 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marktnoonan commented Apr 26, 2022

marktnoonan commented Apr 26, 2022

lmiller1990 commented Apr 26, 2022

marktnoonan left a 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

ryanthemanuel left a comment

Choose a reason for hiding this comment

lmiller1990 commented Apr 21, 2022 •

edited

Loading

cypress bot commented Apr 21, 2022 •

edited

Loading

lmiller1990 Apr 22, 2022 •

edited

Loading

lmiller1990 Apr 22, 2022 •

edited

Loading