fix(unify): Updating reporter to consistently use app-provided "Preferred Editor" dialog #19933

tbiethman · 2022-01-27T19:34:03Z

The reporter package had a few areas where the react-based dialog was still being used to manage file opening. I updated these to emit the appropriate events so that these flows go through graphql and the app's provided dialog.

Also migrating helpers and initial tests to app from existing runner specs. More tests to come; I wanted to get some of the existing helper logic + the dialog fix in before inflating the PR size too much.

User facing changelog

Additional details

One side effect of the reporter component restructuring is that we have a little more control over the tooltip presentation. With these changes, the "Open in IDE" tooltip does not present when the link/element itself says "Open in IDE", as it was kind of redundant. However, I could not find designs to that effect, so we can add those back if we need to.

How has the user experience changed?

Before:

After:

PR Tasks

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

Migrating helpers and initial tests to app from existing runner specs

cypress-bot · 2022-01-27T19:34:05Z

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-01-27T19:39:27Z

Test summary

17897 • 0 • 211 • 0 • 5

Run details


Project	cypress
Status	Passed
Commit	`85ef6b6`
Started	Jan 31, 2022 4:44 PM
Ended	Jan 31, 2022 4:57 PM
Duration	12:07 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	commands/net_stubbing.cy.ts	3
1	network stubbing > waiting and aliasing > should handle aborted requests
2	network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"
3	network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"
	cypress/proxy-logging.cy.ts	2
1	Proxy Logging > request logging > xhr log has response body/status code
2	Proxy Logging > request logging > xhr log has response body/status code

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

tbiethman · 2022-01-28T03:27:35Z

packages/app/cypress/e2e/runner/support/verify-helpers.ts

@@ -0,0 +1,205 @@
+import _ from 'lodash'


The majority of this file was pre-existing in the runner package here. It's been modified here to no longer use runIsolatedCypress, as the spec is responsible for scaffolding and initializing the e2e test. We also no longer spy on the socket for external IDE interactions and intercept mutations/assert DOM instead.

This might change a bit as more tests get ported over.

Nice, definitely worth porting this - it might be a bit of work to figure out how it all works and what it does, but definitely time well spent.

packages/reporter/src/hooks/hooks.tsx

tbiethman · 2022-01-28T03:41:24Z

system-tests/projects/e2e/cypress/support/util.js

@@ -70,12 +56,6 @@ export const verify = (ctx, options) => {
 .should('not.include.text', '__stackReplacementMarker')

 cy.contains('.runnable-err-stack-trace .runnable-err-file-path', openInIdePath.relative)


I left the assertions for the element presence but removed the action assertions. I struggled to find a good way to continue to have coverage for the actions here; any suggestions are welcomed.

tbiethman · 2022-01-28T04:03:13Z

packages/reporter/src/lib/file-opener.tsx

@@ -1,43 +0,0 @@
-import { observer } from 'mobx-react'


This component is no longer needed now that we delegate this workflow through graphql.

tbiethman · 2022-01-28T04:07:14Z

packages/app/cypress/e2e/runner/reporter.errors.cy.ts

@@ -0,0 +1,90 @@
+import { verify } from './support/verify-helpers'


These tests were carried forward from the runner package and modified to be more e2e. There's a lot more where this came from, I'll be adding more in future PRs.

tbiethman · 2022-01-28T04:08:22Z

packages/reporter/cypress/support/utils.ts

- })
- })
-
- describe('when user has not already set opener and opens file', () => {


These workflows and related components are no longer accessible. I think we're good to remove the ui-components/FileOpener and related files at this point? I can go ahead and do that here too if that makes sense.

Yep, as we remove more code we should incrementally delete it from @packages/ui-components. We can't get rid of all that package, since some of it is used in the reporter, but we can get close. The remainder of the components could probably just get collapsed into reporter.

tbiethman · 2022-01-28T04:09:09Z

packages/reporter/cypress/support/utils.ts

- where: availableEditors[4],
- ...file,
- })
+ expect(getRunner().emit).to.be.calledWith('open:file:unified')


We continue to have token coverage here by asserting the correct event gets emitted, but the rest of the coverage will come from app/e2e.

tbiethman · 2022-01-28T04:52:24Z

packages/reporter/src/runnables/runnables.tsx

+ }}>
+ <h3><OpenIcon />Open file in IDE</h3>
+ </a>
+ </OpenFileInIDE>


I'm assuming that we don't want a tooltip here either, but we do want the link styles. So this guy is a little cobbled together.

I think this is okay for now 👍

lmiller1990

Code seems good - I didn't give this a fully expressive test locally yet, I'm guessing you gave it a pretty good try?

lmiller1990 · 2022-01-28T09:56:07Z

packages/app/cypress/e2e/runner/support/verify-helpers.ts

@@ -0,0 +1,205 @@
+import _ from 'lodash'


Nice, definitely worth porting this - it might be a bit of work to figure out how it all works and what it does, but definitely time well spent.

lmiller1990 · 2022-01-28T09:57:21Z

packages/reporter/cypress/support/utils.ts

- })
- })
-
- describe('when user has not already set opener and opens file', () => {


Yep, as we remove more code we should incrementally delete it from @packages/ui-components. We can't get rid of all that package, since some of it is used in the reporter, but we can get close. The remainder of the components could probably just get collapsed into reporter.

packages/reporter/src/lib/file-name-opener.tsx

lmiller1990 · 2022-01-28T09:59:41Z

packages/reporter/src/lib/open-file-in-ide.tsx

- {props.children}
- </span>
- </Tooltip>
+ <span className={props.className} onClick={() => events.emit('open:file:unified', props.fileDetails)}>


Nice, I guess this could become open:file (without unified) potentially?

Yeah I think so, now that we're not needing two separate communication paths. I'll get a follow up PR out to remove the old plumbing and remove the old UI component.

lmiller1990 · 2022-01-28T10:00:16Z

packages/reporter/src/runnables/runnables.tsx

+ }}>
+ <h3><OpenIcon />Open file in IDE</h3>
+ </a>
+ </OpenFileInIDE>


I think this is okay for now 👍

flotwig

LGTM, tested working on Linux too

@lmiller1990

* 10.0-release: fix: restore @lmiller1990's changes feat: styling snapshots (#19972) fix bad merge overrides- GOOD CATCH TYLER! fix(unify): Updating reporter to consistently use app-provided "Preferred Editor" dialog (#19933) fix: move node 17 Check from Binary to CLI (#19977) fix: pass correct spec URL in `cypress run` on Windows (#19890) fix: send click event with `cy.type('{enter}')`. (#19726) feat: detect package manager in wizard (#19960) fix: refactor set specs by specPattern (#19953) chore: Update Chrome (beta) to 98.0.4758.74

fix(unify): Updating reporter to use consistent preferred editor dialog

0c758ff

Migrating helpers and initial tests to app from existing runner specs

tbiethman marked this pull request as draft January 27, 2022 19:35

tbiethman added 2 commits January 27, 2022 14:48

Simplifying reporter tests around IDE launch

5ed5d2a

It'd help to open the stack trace

b839bcd

tbiethman changed the title ~~fix(unify): Updating reporter to use vue preferred editor dialog~~ fix(unify): Updating reporter to consistently use app-provided "Preferred Editor" dialog Jan 27, 2022

tbiethman added 3 commits January 27, 2022 15:45

Scoping util hooks

7572e30

Removing unnecessary spec project files

128dc91

Removing socket emit assertions from system test validation

ee8ace5

tbiethman commented Jan 28, 2022

View reviewed changes

packages/reporter/src/hooks/hooks.tsx Show resolved Hide resolved

tbiethman commented Jan 28, 2022

View reviewed changes

Condensing components further to better control tooltip presentation

3aafc1f

tbiethman commented Jan 28, 2022

View reviewed changes

Updating runnables usage

fb2578b

tbiethman marked this pull request as ready for review January 28, 2022 04:44

tbiethman commented Jan 28, 2022

View reviewed changes

lmiller1990 approved these changes Jan 28, 2022

View reviewed changes

tbiethman self-assigned this Jan 28, 2022

BlueWinds requested a review from flotwig January 28, 2022 16:39

flotwig approved these changes Jan 31, 2022

View reviewed changes

elevatebart added 2 commits January 31, 2022 10:32

Merge branch '10.0-release' into runner-e2e-round-1

db5417b

Merge branch '10.0-release' into runner-e2e-round-1

85ef6b6

tbiethman merged commit dbe9030 into 10.0-release Jan 31, 2022

tbiethman deleted the runner-e2e-round-1 branch January 31, 2022 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(unify): Updating reporter to consistently use app-provided "Preferred Editor" dialog #19933

fix(unify): Updating reporter to consistently use app-provided "Preferred Editor" dialog #19933

tbiethman commented Jan 27, 2022 •

edited

Loading

cypress-bot bot commented Jan 27, 2022

cypress bot commented Jan 27, 2022 •

edited

Loading

tbiethman Jan 28, 2022 •

edited

Loading

lmiller1990 Jan 28, 2022

tbiethman Jan 28, 2022 •

edited

Loading

tbiethman Jan 28, 2022

tbiethman Jan 28, 2022 •

edited

Loading

tbiethman Jan 28, 2022 •

edited

Loading

lmiller1990 Jan 28, 2022

tbiethman Jan 28, 2022

tbiethman Jan 28, 2022

lmiller1990 Jan 28, 2022

lmiller1990 left a comment

lmiller1990 Jan 28, 2022

lmiller1990 Jan 28, 2022

lmiller1990 Jan 28, 2022

tbiethman Jan 31, 2022

lmiller1990 Jan 28, 2022

flotwig left a comment

		@@ -70,12 +56,6 @@ export const verify = (ctx, options) => {
		.should('not.include.text', '__stackReplacementMarker')

		cy.contains('.runnable-err-stack-trace .runnable-err-file-path', openInIdePath.relative)

		@@ -0,0 +1,90 @@
		import { verify } from './support/verify-helpers'

fix(unify): Updating reporter to consistently use app-provided "Preferred Editor" dialog #19933

fix(unify): Updating reporter to consistently use app-provided "Preferred Editor" dialog #19933

Conversation

tbiethman commented Jan 27, 2022 • edited Loading

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Jan 27, 2022

cypress bot commented Jan 27, 2022 • edited Loading

Test summary

Run details

Flakiness

tbiethman Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbiethman Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbiethman Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

tbiethman Jan 28, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flotwig left a comment

Choose a reason for hiding this comment

tbiethman commented Jan 27, 2022 •

edited

Loading

cypress bot commented Jan 27, 2022 •

edited

Loading

tbiethman Jan 28, 2022 •

edited

Loading

tbiethman Jan 28, 2022 •

edited

Loading

tbiethman Jan 28, 2022 •

edited

Loading

tbiethman Jan 28, 2022 •

edited

Loading