chore(server): convert modes/run to typescript #23536

flotwig · 2022-08-24T20:41:46Z

Closes

User facing changelog

n/a

Additional details

Was working on refactoring the video recording code for Safari (WebKit) Support #6422 and was finally driven insane by the number of floating untyped variables in modes/run.
I added types with minimal refactoring - it's still a mess, but it's a typed mess.
GitHub's built in diff tool fails to recognize the rename of run.js -> run.ts if you view all changes together. To make it easier to review, here are the distinct commits and what they do:
- 11ecebd adds types and fixes type errors
- 98e69d0 removes a unit-level run spec that has not been running for a while now, and has been superseded by e2e/integration tests
- d06d529 removes all run exports except run() itself. moving everything off of module.exports and this caused new type errors since more type checking took place, this commit fixes those errors.

Steps to test

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?

cypress-bot · 2022-08-24T20:41:48Z

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-25T00:08:33Z

Test summary

41474 • 0 • 1434 • 0 • 14

Run details


Project	cypress
Status	Passed
Commit	`a75ec8a`
Started	Aug 25, 2022 4:36 PM
Ended	Aug 25, 2022 4:52 PM
Duration	16:15 💡
OS	Linux Debian - 11.3
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	cypress/e2e/commands/net_stubbing.cy.ts		5
1	network stubbing > intercepting request > can delay with deprecated delayMs param
2	network stubbing > intercepting request > can delay with deprecated delayMs param
3	network stubbing > intercepting request > can delay with deprecated delayMs param
4	... > with `times` > only uses each handler N times
5	network stubbing > intercepting response > can 'delay' a proxy response using Promise.delay
This comment includes only the first 5 flaky tests. See all 14 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

no need to import cypress fix .tap video fixes remove wk mention more async cleanup fix cypress_spec fix exitEarly

flotwig · 2022-08-25T00:49:03Z

packages/server/test/integration/cypress_spec.js

@@ -338,8 +339,11 @@ describe('lib/cypress', () => {
      await clearCtx()

      sinon.stub(electron.app, 'on').withArgs('ready').yieldsAsync()
-      sinon.stub(runMode, 'waitForSocketConnection').resolves()
-      sinon.stub(runMode, 'listenForProjectEnd').resolves({ stats: { failures: 0 } })
+      globalThis.CY_TEST_MOCK = {


So, I introduced this pattern because we have a lot of integration test coverage here that does deep mocking of (what was previously on) module.exports. I know this is an unfortunate way to handle mocking, but I felt it was the lesser of evils... the other options being to export everything to make it stubbable and call methods off module.exports, or some kinda proxyquire hackery. Open to better ideas here.

I really question the value of some of these tests, where we mock 90% of the world to test some random method... I almost wonder if we collapse ~100 unit tests into ~5 system tests and just aggressively delete some of these. I think a lot of these are from the days prior to system-tests existing?

If we have really complex unit level logic, stick it in a pure function and unit test that, so we don't need so much mocking/stubbing.

Agreed. I didn't want to have to dive into figuring out what edge cases are/aren't covered in these integration tests versus other tests for this PR, but we do have way too many tests like this.

flotwig · 2022-08-25T00:50:01Z

packages/server/test/integration/cypress_spec.js

-      sinon.stub(runMode, 'waitForSocketConnection').resolves()
-
-      sinon.stub(runMode, 'waitForBrowserToConnect').resolves({
-        stats: {


this object stub shape seems to have been a typo, which is why it was removed

lmiller1990 · 2022-08-25T01:04:37Z

packages/server/lib/modes/run.ts

+  }
+}
+
+async function runSpecs (options: { config: Cfg, browser: Browser, sys: any, headed: boolean, outputPath: string, specs: SpecWithRelativeRoot[], specPattern: string | RegExp | string[], beforeSpecRun?: BeforeSpecRun, afterSpecRun?: AfterSpecRun, runUrl?: string, parallel?: boolean, group?: string, tag?: string, testingType: TestingType, quiet: boolean, project: Project, onError: (err: Error) => void, exit: boolean, socketId: string, webSecurity: boolean, projectRoot: string } & Pick<Cfg, 'video' | 'videoCompression' | 'videosFolder' | 'videoUploadOnPasses'>) {


Ahhh yes awesome, we need this so desperately. Every time I tried to make this into TS, I realized how much work it was and abandoned it - definitely time well spent to figure out what these types are.

Yeah, there's still about 4 billion de-structured variables too many in this file, but at least this will enable us to more confidently refactor in the future.

lmiller1990 · 2022-08-25T01:09:51Z

packages/types/src/config.ts

@@ -30,7 +30,7 @@ export interface FullConfig extends Partial<Cypress.RuntimeConfigOptions & Cypre
 // and are required when creating a project.
 export type ReceivedCypressOptions =
  Pick<Cypress.RuntimeConfigOptions, 'hosts' | 'projectName' | 'clientRoute' | 'devServerPublicPathRoute' | 'namespace' | 'report' | 'socketIoCookie' | 'configFile' | 'isTextTerminal' | 'isNewProject' | 'proxyUrl' | 'browsers' | 'browserUrl' | 'socketIoRoute' | 'arch' | 'platform' | 'spec' | 'specs' | 'browser' | 'version' | 'remote'>
-  & Pick<Cypress.ResolvedConfigOptions, 'chromeWebSecurity' | 'supportFolder' | 'experimentalSourceRewriting' | 'fixturesFolder' | 'reporter' | 'reporterOptions' | 'screenshotsFolder' | 'supportFile' | 'baseUrl' | 'viewportHeight' | 'viewportWidth' | 'port' | 'experimentalInteractiveRunEvents' | 'userAgent' | 'downloadsFolder' | 'env' | 'excludeSpecPattern' | 'specPattern' | 'experimentalSessionAndOrigin' | 'experimentalModifyObstructiveThirdPartyCode'> // TODO: Figure out how to type this better.
+  & Pick<Cypress.ResolvedConfigOptions, 'chromeWebSecurity' | 'supportFolder' | 'experimentalSourceRewriting' | 'fixturesFolder' | 'reporter' | 'reporterOptions' | 'screenshotsFolder' | 'supportFile' | 'baseUrl' | 'viewportHeight' | 'viewportWidth' | 'port' | 'experimentalInteractiveRunEvents' | 'userAgent' | 'downloadsFolder' | 'env' | 'excludeSpecPattern' | 'specPattern' | 'experimentalSessionAndOrigin' | 'experimentalModifyObstructiveThirdPartyCode' | 'video' | 'videoCompression' | 'videosFolder' | 'videoUploadOnPasses' | 'resolvedNodeVersion' | 'resolvedNodePath' | 'trashAssetsBeforeRuns'> // TODO: Figure out how to type this better.


One day this will be Pick<all the things>. I wonder if this is useful? I think I wrote this originally, when it had like 2-3 properties in Pick... it's growing with every PR!

I think the original intent was "These are the options we use in this part of the code" but we might have outgrown it.

Yeah, I was questioning this also. It doesn't seem like we would have only some ResolvedConfigOptions and not others at any point.

lmiller1990

Thanks for this - I know it's still draft but going to follow along, this will have very high long term value - working in run.js is kind of like stumbling around in the dark at the moment :|

tbiethman

LGTM. I'll second the thanks for this, it will be a big help going forward.

packages/server/lib/modes/run.ts

emilyrohrbough · 2022-08-25T16:28:09Z

packages/types/src/server.ts

@@ -46,7 +47,7 @@ export interface AutomationMiddleware {
  onBeforeRequest?: OnRequestEvent | null
  onRequest?: OnRequestEvent | null
  onResponse?: NullableMiddlewareHook
-  onAfterResponse?: NullableMiddlewareHook
+  onAfterResponse?: ((eventName: string, data: any, resp: any) => void) | null


I'd be nice if we someday remove the null states and leveraged undefined

emilyrohrbough · 2022-08-25T16:28:53Z

packages/server/lib/modes/run.ts

+    // electron >= 5.0.0 will exit the app if all browserwindows are closed,
+    // this is obviously undesirable in run mode
+    // https://github.com/cypress-io/cypress/pull/4720#issuecomment-514316695
+    app.on('window-all-closed', () => {


I wonder if this is still true

Yeah, it should be. This was an intentional change, since most Electron apps should have 0 processes left when all the windows are closed. We're the exception to their rule

Here's the underlying Electron code that handles this, for reference: https://github.com/electron/electron/blob/d44a187d0b226800fe0cb4f7a0d2b36c871b27cd/lib/browser/init.ts#L172

Co-authored-by: Emily Rohrbough <[email protected]>

flotwig added 2 commits August 24, 2022 16:40

chore(server): convert modes/run to typescript

11ecebd

remove skipped run spec

98e69d0

fix inter-function type-checking, remove this., fix type errors

d06d529

no need to import cypress fix .tap video fixes remove wk mention more async cleanup fix cypress_spec fix exitEarly

flotwig force-pushed the ts-run-mode branch from bb20e66 to d06d529 Compare August 25, 2022 00:46

flotwig commented Aug 25, 2022

View reviewed changes

lmiller1990 reviewed Aug 25, 2022

View reviewed changes

flotwig marked this pull request as ready for review August 25, 2022 01:08

flotwig requested review from tbiethman and lmiller1990 August 25, 2022 01:08

lmiller1990 reviewed Aug 25, 2022

View reviewed changes

flotwig requested a review from lmiller1990 August 25, 2022 01:12

tbiethman approved these changes Aug 25, 2022

View reviewed changes

emilyrohrbough reviewed Aug 25, 2022

View reviewed changes

packages/server/lib/modes/run.ts Outdated Show resolved Hide resolved

emilyrohrbough reviewed Aug 25, 2022

View reviewed changes

packages/server/lib/modes/run.ts Outdated Show resolved Hide resolved

emilyrohrbough reviewed Aug 25, 2022

View reviewed changes

packages/server/lib/modes/run.ts Outdated Show resolved Hide resolved

emilyrohrbough reviewed Aug 25, 2022

View reviewed changes

emilyrohrbough approved these changes Aug 25, 2022

View reviewed changes

Apply suggestions from code review

a75ec8a

Co-authored-by: Emily Rohrbough <[email protected]>

flotwig merged commit bd0d38f into develop Aug 25, 2022

flotwig deleted the ts-run-mode branch August 25, 2022 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(server): convert modes/run to typescript #23536

chore(server): convert modes/run to typescript #23536

flotwig commented Aug 24, 2022 •

edited

Loading

cypress-bot bot commented Aug 24, 2022

cypress bot commented Aug 25, 2022 •

edited

Loading

flotwig Aug 25, 2022

lmiller1990 Aug 25, 2022

flotwig Aug 25, 2022

flotwig Aug 25, 2022 •

edited

Loading

lmiller1990 Aug 25, 2022

flotwig Aug 25, 2022

lmiller1990 Aug 25, 2022

flotwig Aug 25, 2022

lmiller1990 left a comment

tbiethman left a comment

emilyrohrbough Aug 25, 2022

emilyrohrbough Aug 25, 2022

flotwig Aug 25, 2022

tbiethman Aug 25, 2022

chore(server): convert modes/run to typescript #23536

chore(server): convert modes/run to typescript #23536

Conversation

flotwig commented Aug 24, 2022 • edited Loading

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Aug 24, 2022

cypress bot commented Aug 25, 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

flotwig Aug 25, 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

tbiethman 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

flotwig commented Aug 24, 2022 •

edited

Loading

cypress bot commented Aug 25, 2022 •

edited

Loading

flotwig Aug 25, 2022 •

edited

Loading