chore: Intregrate runner packages. #23028

sainthkh · 2022-08-01T02:16:44Z

Related with Delete old runner code #21689

User facing changelog

N/A. It's an internal code cleanup.

Additional details

Why was this change necessary? => These are mostly-unused legacy code.
What is affected by this change? => N/A
Any implementation details to explain? => I migrated runner-shared and runner-ct into runner and removed unused scss files and react components.

Steps to test

packages folder structure has been changed. Run all tests to make sure that nothing has changed.

How has the user experience changed?

N/A. New contributors might not be confused with the legacy codes.

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?

cypress-bot · 2022-08-01T02:16:47Z

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-08-01T03:11:28Z

Test summary

37828 • 0 • 473 • 0 • 10

Run details


Project	cypress
Status	Passed
Commit	`993f918`
Started	Aug 2, 2022 1:42 AM
Ended	Aug 2, 2022 1:57 AM
Duration	15:33 💡
OS	Linux Debian - 11.3
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	cypress/e2e/commands/xhr.cy.js		5
1	... > logs request + response headers
2	... > logs Method, Status, URL, and XHR
3	... > logs response
4	... > logs request + response headers
5	... > logs Method, Status, URL, and XHR
This comment includes only the first 5 flaky tests. See all 10 flaky tests in the Cypress Dashboard.

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

lmiller1990

Left a few comments/questions, happy to approve once they've been replied too - nice job on this, great to see some of this getting cleaned up!

packages/server/lib/controllers/runner.ts

lmiller1990 · 2022-08-02T04:37:30Z

.github/CODEOWNERS

@@ -40,8 +40,6 @@
 /packages/rewriter/  @cypress-io/end-to-end
 /packages/root/  @cypress-io/end-to-end
 /packages/runner/  @cypress-io/end-to-end
-/packages/runner-ct/  @cypress-io/component-testing
-/packages/runner-shared/  @cypress-io/end-to-end
 /packages/server/  @cypress-io/end-to-end
 /packages/socket/  @cypress-io/end-to-end
 /packages/static/  @cypress-io/end-to-end


FYI some context - we ideally should delete ALL of runner/runner-shared, and bundle the entire app with the Vite stack.

The reason we are stuck like this is explained a little here: #20663 (comment)

Basically some of the code in driver and possibly reporter are not really ESM compatible - webpack does something different to Vite to resolve the modules, which is why it works in webpack, but we cannot import to Vite.

Not sure if this is something you are looking to investigate more @sainthkh but if you do please let me know, I'd like to follow along/help out - I'd really like to get away from this "inject webpack bundle" mess at some point, but it's quite hard. Likely making driver 100% ESM (including dependencies) would be a good first step.

Any thoughts on this?

It seems that we have 4 things to do to delete the runner pacakge.

Migrate dom. -> I think driver/src/dom might be a good place or move it to app directly.

Reimplement Studio -> Cypress Studio Reimplementation Spike #22870

ESM-ify driver -> I agree it's the biggest problem.

Let reporter compile its own styles. -> Move runner/webpack.config.ts to reporter.

Dom -> are you going do to this here?
Studio -> I am looking into this now
ESMify driver -> agree, hard
Reporter styles -> do this separately I think?

I read the dom code and it seems that it's better to be a separate PR.

It currently renders highlight by

Creating the container node with jQuery.

Rendering the Highlight DOM with React.

I can simply migrate dom to app and use unified-runner. But I don't think it's not our goal. It's just hiding and postponing.

To get migrating dom done, I need to do these things:

Remove JQuery from dom and dimensions.

Move the current JQuery version of dimensions to driver/src/dom because it's used in driver/src/dom/blackout.

Migrate Highlight from react to vue with our Tooltip component.

Migrate dom functions to app/runner/aut-iframe. Leave studio-related functions.

Clean up unified-runner dom and CypressJQuery. They're only used inside aut-iframe. I guess they can be removed safely from unified-runner.

I think it's a big change and outside the scope of this PR.

As for reporter styles, it's safe to do it after ESM-ifying driver. We're trying to remove webpack. I don't think we don't need to create one more webpack.config.ts now.

I opened the new PR #23187 to migrate dom.js. It's currently empty because I'm now reading and planning.

.gitignore

emilyrohrbough

We think alike! I love it. I started cleaning this up last week while waiting on test results because it was bugging me! I'll drop a few comments on things I noticed while cleaning this up.

emilyrohrbough · 2022-08-02T14:21:13Z

packages/runner/package.json

@@ -24,6 +24,7 @@
    "@packages/rewriter": "0.0.0-development",


Need to add in the test script to run the mocha tests that were in runner-shared

and need to move the mocha test setup over as well - the test is in src/studio

also "@packages/network": "0.0.0-development", can be dropped

Currently, runner-shared unit tests are ignored.

cypress/package.json

Line 53 in f69afb5

"test": "yarn lerna exec yarn test --scope cypress --scope \"'@packages/{config,errors,data-context,electron,extension,https-proxy,launcher,net-stubbing,network,proxy,rewriter,socket}'\"",

And it fails in the current develop branch.

And the only the file that exists is studio-recorder.spec.js. It's there for reference purpose and it'll be removed soon with #22870.

I checked the code base and removed network, socket, and rewriter.

packages/runner/package.json

packages/runner-ct/webpack.config.ts

lmiller1990

Nice cleanup! Going to approve, but also happy to re-review if you move the DOM code to app.

Nice list of TODOs here: #23028 (comment)

lmiller1990 · 2022-08-09T00:48:12Z

Are you still working on this @sainthkh or are we good to re-review and merge if green?

sainthkh · 2022-08-10T01:24:13Z

@lmiller1990 I think it's done. I'll take care of the dom issue at #23187.

lmiller1990 · 2022-08-10T23:58:28Z

run-reporter seems broken. Last blocker @sainthkh 🤔

sainthkh · 2022-08-11T02:37:39Z

@lmiller1990 It's happening because there are 4 test files inside reporter(commands/command.cy.tsx, hooks/hooks.cy.tsx, lib/tag.cy.tsx, test/test.cy.tsx), but there are 7 machines to run them. 3 of them don't have work to do. I opened an issue about this misleading message a few months ago at #22328.

I intentionally didn't fix this problem at #22326 because I thought the problem is more at the ci setting side (comment).

There are a few solutions for this problem:

Change the CI setting from 7 to 2 or 3. -> I think it's better because we need less power and it's unlikely that we might need more than 2 or 3 machines for the tests.
Return nothing when there is no test file. -> We need to fix the ci script. Maybe it's a different issue and open a new PR.

lmiller1990 · 2022-08-11T03:42:18Z

Let's just take it down to parallelism: 2 for now. For such a small number of specs, we probably don't need it at all...

sainthkh force-pushed the issue-21689 branch from 02a4378 to 993f918 Compare August 2, 2022 01:35

sainthkh marked this pull request as ready for review August 2, 2022 01:53

sainthkh requested review from a team as code owners August 2, 2022 01:53

lmiller1990 mentioned this pull request Aug 2, 2022

Cypress Studio Reimplementation Spike #22870

Closed

lmiller1990 reviewed Aug 2, 2022

View reviewed changes

emilyrohrbough reviewed Aug 2, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

emilyrohrbough reviewed Aug 2, 2022

View reviewed changes

packages/runner/package.json Show resolved Hide resolved

emilyrohrbough reviewed Aug 2, 2022

View reviewed changes

packages/runner-ct/webpack.config.ts Show resolved Hide resolved

emilyrohrbough reviewed Aug 2, 2022

View reviewed changes

packages/runner-ct/webpack.config.ts Show resolved Hide resolved

sainthkh added 9 commits August 3, 2022 10:03

chore: Intregrate runner packages.

54b73b5

Remove unnecessary Studio react files.

e836b14

Remove unnecessary gif

1c4e85e

runner-shared to runner-ct.

73fdfb4

fix path.

8fab511

fix package.json

a00e2aa

Remove scss files from runner-ct

e636e95

Remove runner-ct

d1c7754

Remove runner-shared and runner-ct comments.

e1b887f

lmiller1990 self-requested a review August 3, 2022 01:04

lmiller1990 approved these changes Aug 3, 2022

View reviewed changes

Feedback

166cc8a

sainthkh force-pushed the issue-21689 branch from 993f918 to 166cc8a Compare August 3, 2022 02:45

Merge branch 'develop' into issue-21689

73fc73c

emilyrohrbough approved these changes Aug 10, 2022

View reviewed changes

lmiller1990 added 2 commits August 11, 2022 13:39

chore: reduce parallelism for reporter-componen-tests

a4b4f7f

chore: reduce paralelleism

cccd76e

lmiller1990 merged commit 2d119ad into cypress-io:develop Aug 11, 2022

lmiller1990 mentioned this pull request Aug 11, 2022

Re-introduce Cypress Studio for E2E #23230

Closed

sainthkh mentioned this pull request Sep 18, 2022

Delete old runner code #21689

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Intregrate runner packages. #23028

chore: Intregrate runner packages. #23028

sainthkh commented Aug 1, 2022 •

edited

Loading

cypress-bot bot commented Aug 1, 2022

cypress bot commented Aug 1, 2022 •

edited

Loading

lmiller1990 left a comment

lmiller1990 Aug 2, 2022 •

edited

Loading

sainthkh Aug 3, 2022

lmiller1990 Aug 3, 2022

sainthkh Aug 3, 2022

sainthkh Aug 8, 2022

emilyrohrbough left a comment

emilyrohrbough Aug 2, 2022 •

edited

Loading

emilyrohrbough Aug 2, 2022

sainthkh Aug 3, 2022

lmiller1990 left a comment

lmiller1990 commented Aug 9, 2022

sainthkh commented Aug 10, 2022

lmiller1990 commented Aug 10, 2022

sainthkh commented Aug 11, 2022

lmiller1990 commented Aug 11, 2022

chore: Intregrate runner packages. #23028

chore: Intregrate runner packages. #23028

Conversation

sainthkh commented Aug 1, 2022 • edited Loading

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Aug 1, 2022

cypress bot commented Aug 1, 2022 • edited Loading

Test summary

Run details

Flakiness

lmiller1990 left a comment

Choose a reason for hiding this comment

lmiller1990 Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

sainthkh Aug 3, 2022

Choose a reason for hiding this comment

lmiller1990 Aug 3, 2022

Choose a reason for hiding this comment

sainthkh Aug 3, 2022

Choose a reason for hiding this comment

sainthkh Aug 8, 2022

Choose a reason for hiding this comment

emilyrohrbough left a comment

Choose a reason for hiding this comment

emilyrohrbough Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

emilyrohrbough Aug 2, 2022

Choose a reason for hiding this comment

sainthkh Aug 3, 2022

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

lmiller1990 commented Aug 9, 2022

sainthkh commented Aug 10, 2022

lmiller1990 commented Aug 10, 2022

sainthkh commented Aug 11, 2022

lmiller1990 commented Aug 11, 2022

sainthkh commented Aug 1, 2022 •

edited

Loading

cypress bot commented Aug 1, 2022 •

edited

Loading

lmiller1990 Aug 2, 2022 •

edited

Loading

emilyrohrbough Aug 2, 2022 •

edited

Loading