feat: add subscription for specs changing #20657

ryanthemanuel · 2022-03-17T01:34:20Z

Closes UNIFY-1347 and UNIFY-1275

User facing changelog

Ensure that when the spec pattern changes inside of the cypress config, we start watching the new spec pattern in the file system for changes

Additional details

In addition to fixing the spec pattern watching logic, we are also converting specs to use the gql subscription system. In the process of working on this, I realized that we had a couple of race conditions involving the subscription async iterator, where we might get events while we're not actually awaiting them yet and thus they get lost. Also, when stepping through the graphql-ws code, I realized that when we are "returning" out of the async iterator, so I am fixing that as well by ensuring we always resolve the promise we are waiting on in the subscription.

How has the user experience changed?

PR Tasks

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

cypress-bot · 2022-03-17T01:34:23Z

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-03-17T16:09:33Z

Test summary

17784 • 0 • 217 • 0 • 2

Run details


Project	cypress
Status	Passed
Commit	`574a705`
Started	Mar 22, 2022 4:29 PM
Ended	Mar 22, 2022 4:42 PM
Duration	12:32 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	commands/xhr.cy.js	1
1	... > no status when request isnt forced 404
	cypress/proxy-logging.cy.ts	1
1	Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second

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

* add an extra test for migration * refactor: use async tools to deal with files Co-authored-by: Barthélémy Ledoux <bart@cypress.io>

@JessicaSachs

* feat: rename __socket.io -> __socket, add __socket-graphql for graphql-ws * fix: remove currentTestingType from Query type * feat: add subscription to refresh when auth state changes * Update packages/frontend-shared/src/gql-components/HeaderBarContent.vue * PR review updates, add docs, tests for authChange subscription * remove login/logout toApp/toLaunchpad calls * remove login/logout/devChange toApp/toLaunchpad calls * fix, getCtx lazily to fix proxy performance spec * Fix, thanks @JessicaSachs * test fixes, remove flash of missing content in wizard Co-authored-by: Ryan Manuel <ryanm@cypress.io> Co-authored-by: Jess <jess@jessicasachs.io> Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>

* remove pluginsFile references * fix all tests relying on this system test * fix broken tests * fix more broken tests * one more * relative path * pr feedbkac * fix vite-dev-server broken configuration * PR feedback on error message * update errors snaphsot * bring back error in gql * fix snapshot * fix snapshot. * fix merge issues Co-authored-by: ElevateBart <ledouxb@gmail.com>

…is removed (#20515) * add terminal warning for experimental studio * add to errors old * add warnings to main * debug onWarning callback * show error in open mode * update snapshot * update tests * validate config earlier * remove from types and schema * use on link * refactor getError * update types Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com> * add unit test for validateNoBreakingConfigLaunchpad * fix types * merge test files * update types Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>

…e specific" This reverts commit c002011.

ZachJW34 · 2022-03-18T21:59:33Z

packages/app/cypress/e2e/subscriptions/specChange-subscription.cy.ts

+        .should('contain', 'new-file.spec.js')
+      })
+
+      it('handles removing the last file', () => {


Can this test be expanded so as to verify that once you get redirected to the "No Specs Page", adding a file takes you back to the SpecsList. We fixed that bug by moving the subscription up a level but would be good to cover that case.

@ZachJW34, the test before this one accomplishes that.

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

ryanthemanuel · 2022-03-21T16:01:20Z

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

+        if (done) {
+          return { done: true, value: undefined }
+        }
+
        if (!hasSentInitial && sendInitial) {
          hasSentInitial = true

          return { done: false, value: {} }


@tgriesser are the values that we're emitting here being used? As I was working through this logic I was trying to figure out if {} was appropriate here, or undefined?

The value here is what will flow into the first argument of the resolve for the subscription. So if we want to have a more targeted subscription where we're not looking to merge with existing types, like notifying where the type is dependent on an individual file change (where there is no initial value), we can set sendInitial to false, and just use the values that are sent into the emitter to determine the resolved value. Here's a rough example of what I mean:

type SpecEventType { ADD CHANGE REMOVE } type SpecEvent { event: SpecEventType filePath: String fileParts: FileParts } type Subscription { """Fired whenever a spec changes""" specEvent: SpecEvent }

subscribe: (source, args, ctx) => ctx.subscribeTo('specEvt', false) resolve: (specEvt, args, ctx) => { return {...specEvt, fileParts: {absolute: specEvt.filePath}} }

on.all('evt', (event, filePath) => { ctx.emitter.specEvt({ event, filePath }) }

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

tgriesser

Added a few comments, but otherwise looks pretty solid! Just kicked off the CI pipeline again to see if it will complete

tgriesser · 2022-03-21T18:01:21Z

guides/graphql-subscriptions.md

@@ -81,7 +81,8 @@ useSubscription({ query: OnSpecChangeDocument }, (prev, next) => {
 - [API Docs for useSubscription](https://formidable.com/open-source/urql/docs/api/vue/#usesubscription)
 - [Subscriptions overview](https://formidable.com/open-source/urql/docs/advanced/subscriptions/)

- One thing to be aware of, is the subscription is only mounted/responded to when the containing component is mounted on the page.
+One thing to be aware of, is the subscription is only mounted/responded to when the containing component is mounted on the page. Therefore, one rule of thumb when using subscriptions is to ensure that they are declared in the subscription dependent view that is highest in the hierarchy so that the subscription can be active when it needs to be. This location is often alongside a query rather than a fragment.
+


Great addition here!

tgriesser · 2022-03-21T18:03:07Z

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

+      throw: async (error: Error) => {
+        throw error


Does adding throw here make a difference? Or was this more useful while debugging the timing issue with the deferreds?

This was to complete the expected type for the async generator. From what I could tell in the graphql-ws an async generator seemed to be what they were expecting if you were going to expose return like we are:

https://github.com/enisdenjo/graphql-ws/blob/7649ad739291a6bfb031bdc4b3487291568fe8a8/src/server.ts#L827-L828

Declaring it as an async generator like I am now, allows me to iterate like I am in the test i wrote but it also necessitates adding the throw function.

tgriesser · 2022-03-21T18:03:30Z

packages/server/lib/server-ct.ts

@@ -21,7 +21,7 @@ export class ServerCt extends ServerBase<SocketCt> {
      this.server.on('connect', this.onConnect.bind(this))
      this.server.on('upgrade', (req, socket, head) => this.onUpgrade(req, socket, head, socketIoRoute))

-      graphqlWS(this.server, `${socketIoRoute}-graphql`)
+      this._graphqlWS = graphqlWS(this.server, `${socketIoRoute}-graphql`)


Nice catch!

tbiethman · 2022-03-21T20:10:19Z

packages/app/cypress/e2e/subscriptions/specChange-subscription.cy.ts

+  if (Cypress.platform === 'win32') return posixPath.replaceAll('/', '\\')
+
+  return posixPath
+}


This helper has been propagated a few times already, but you could import it from here instead: https://github.com/cypress-io/cypress/blob/10.0-release/packages/app/src/paths.ts

574a705 (#20657)

tbiethman · 2022-03-21T20:12:12Z

packages/app/cypress/e2e/subscriptions/specChange-subscription.cy.ts

+    supportFile: false,
+  },
+}`)
+        }, { path: getPathForPlatform('cypress/e2e/dom-list.spec.js') })


Doesn't look like the path is being used here (or with the other config writes)

574a705 (#20657)

tbiethman

Works good! My comments are small.

Not sure if you've done much testing on windows, but I'm curious if this might have an impact on https://cypress-io.atlassian.net/browse/UNIFY-1221 as well.

ryanthemanuel and others added 19 commits March 18, 2022 10:54

rebasing

82501fe

Add tests, doc, and move the location of the subscription

691ce00

chore: improve migration flow test coverage (#20576)

fd02ee4

* add an extra test for migration * refactor: use async tools to deal with files Co-authored-by: Barthélémy Ledoux <bart@cypress.io>

refactor: make config package type-safe (#20590)

6843af8

Move default slowtestthreshold config value to be testing type specific

c3e3b08

Revert "Move default slowtestthreshold config value to be testing typ…

47aecab

…e specific" This reverts commit c002011.

Fix system test

fd29f65

Remove additional references to deleted schema file

c95eb19

Re-add accidentally removed schema reference

00402ed

Fix merge conflicts

111af49

Fix build

c0e3705

Fix tests

051d9e2

Fix order of watching specs

a1d5cf5

Fix timing issue with subscriptions

caac0bc

Clean up graphql subscriptions

2553c44

Fix timing issue with subscriptions

a17c97f

ryanthemanuel force-pushed the ryanm/feat/subscriptions-spec-change branch from 9dc99f8 to a17c97f Compare March 18, 2022 16:18

ryanthemanuel added 3 commits March 18, 2022 11:24

Cleanup merge

9157aae

Fix test

d88ba81

Add doc

1162def

ZachJW34 reviewed Mar 18, 2022

View reviewed changes

add test for async iterator

31bea03

ryanthemanuel commented Mar 21, 2022

View reviewed changes

ryanthemanuel added 2 commits March 21, 2022 11:38

Slight refactoring

110698e

Merge branch '10.0-release' into ryanm/feat/subscriptions-spec-change

33e8796

ryanthemanuel marked this pull request as ready for review March 21, 2022 16:48

ryanthemanuel requested review from ZachJW34, tgriesser, estrada9166 and tbiethman March 21, 2022 16:50

ryanthemanuel commented Mar 21, 2022

View reviewed changes

packages/data-context/src/actions/DataEmitterActions.ts Show resolved Hide resolved

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

dd69258

tgriesser previously approved these changes Mar 21, 2022

View reviewed changes

ZachJW34 previously approved these changes Mar 21, 2022

View reviewed changes

tbiethman reviewed Mar 21, 2022

View reviewed changes

tbiethman previously approved these changes Mar 21, 2022

View reviewed changes

tgriesser and others added 2 commits March 22, 2022 09:53

Merge branch '10.0-release' into ryanm/feat/subscriptions-spec-change

c3c321e

PR comments

574a705

ryanthemanuel dismissed stale reviews from tbiethman, ZachJW34, and tgriesser via 574a705 March 22, 2022 14:22

ryanthemanuel requested review from tgriesser, tbiethman and ZachJW34 March 22, 2022 14:22

tgriesser approved these changes Mar 22, 2022

View reviewed changes

tbiethman approved these changes Mar 22, 2022

View reviewed changes

ryanthemanuel changed the title ~~feat: add subscription for specs changing~~ feat: add subscription for specs changing. Mar 22, 2022

ryanthemanuel changed the title ~~feat: add subscription for specs changing.~~ feat: add subscription for specs changing Mar 22, 2022

ryanthemanuel merged commit 111949a into 10.0-release Mar 22, 2022

ryanthemanuel deleted the ryanm/feat/subscriptions-spec-change branch March 22, 2022 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add subscription for specs changing #20657

feat: add subscription for specs changing #20657

ryanthemanuel commented Mar 17, 2022 •

edited

Loading

cypress-bot bot commented Mar 17, 2022

cypress bot commented Mar 17, 2022 •

edited

Loading

ZachJW34 Mar 18, 2022

ryanthemanuel Mar 21, 2022

ryanthemanuel Mar 21, 2022

tgriesser Mar 21, 2022

tgriesser left a comment

tgriesser Mar 21, 2022

tgriesser Mar 21, 2022

ryanthemanuel Mar 21, 2022

tgriesser Mar 21, 2022

tbiethman Mar 21, 2022

ryanthemanuel Mar 22, 2022

tbiethman Mar 21, 2022 •

edited

Loading

ryanthemanuel Mar 22, 2022

tbiethman left a comment

feat: add subscription for specs changing #20657

feat: add subscription for specs changing #20657

Conversation

ryanthemanuel commented Mar 17, 2022 • edited Loading

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Mar 17, 2022

cypress bot commented Mar 17, 2022 • edited Loading

Test summary

Run details

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

tgriesser 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbiethman Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbiethman left a comment

Choose a reason for hiding this comment

ryanthemanuel commented Mar 17, 2022 •

edited

Loading

cypress bot commented Mar 17, 2022 •

edited

Loading

tbiethman Mar 21, 2022 •

edited

Loading