Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix issues with Cypress.require() and TypeScript #25931

Merged
merged 39 commits into from
Mar 8, 2023

Conversation

chrisbreiding
Copy link
Contributor

@chrisbreiding chrisbreiding commented Feb 23, 2023

Additional details

The root of the fix is two-fold:

  • Moving the cy.origin() callback preprocessing into the child process, so it's the same as preprocessing the spec file itself.
  • Using the user-installed version of the typescript package instead of our own, which actually doesn't exist in the production build of Cypress. This is also the same way it works when preprocessing the spec file itself.

Steps to test

Set up a test project with a TypeScript spec file and use Cypress.require() within cy.origin():

describe('Some test', (): void => {
  it('should work', (): void => {
     cy.origin('https://example.com', (): void => {
        const test = Cypress.require('./test.js');
        cy.log('test', test);
     });
  });
});

How has the user experience changed?

PR Tasks

@cypress
Copy link

cypress bot commented Feb 28, 2023

27 flaky tests on run #44611 ↗︎

0 26874 1281 0 Flakiness 27

Details:

fix test
Project: cypress Commit: af35e9019e
Status: Passed Duration: 19:19 💡
Started: Mar 8, 2023 8:35 PM Ended: Mar 8, 2023 8:54 PM
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-electron

View Output Video

Test Artifacts
... > correctly returns currentRetry Output Video
... > correctly returns currentRetry Output Video
... > correctly returns currentRetry Output Video
Flakiness  commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-firefox

View Output Video

Test Artifacts
network stubbing > intercepting request > can delay and throttle a StaticResponse Output
Flakiness  e2e/origin/navigation.cy.ts • 1 flaky test • 5x-driver-firefox

View Output Video

Test Artifacts
... > establishes foobar spec bridge Output
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-firefox

View Output Video

Test Artifacts
... > correctly returns currentRetry Output
... > correctly returns currentRetry Output
... > correctly returns currentRetry Output
Flakiness  create-from-component.cy.ts • 1 flaky test • app-e2e

View Output Video

Test Artifacts
... > runs generated spec Output Screenshots Video

The first 5 flaky specs are shown, see all 17 specs in Cypress Cloud.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@@ -0,0 +1,67 @@
const md5 = require('md5')
Copy link
Contributor Author

@chrisbreiding chrisbreiding Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this file into the child process. The main reason for the content changes are converting it from typescript to vanilla JS, since we currently don't have the processing set up for TS within the child process.

@@ -14,6 +14,7 @@ const resolve = require('../../util/resolve')
const browserLaunch = require('./browser_launch')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the changes in here was just moving a couple methods around so that the order made a little more sense (to me at least) when I was updating the tests for this. The only thing I added were the lines referencing _process:cross:origin:callback.

@@ -1,433 +1,278 @@
require('../../../spec_helper')
Copy link
Contributor Author

@chrisbreiding chrisbreiding Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these tests had been skipped for over a year, so I unskipped them, fixed the existing ones, and added new ones for my changes to run_plugins.js. It doesn't have 100% test coverage of run_plugins.js (I didn't add tests for any functionality I didn't add or change), but it's a step in the right direction.

@@ -1 +0,0 @@
!node_modules
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This origin-dependencies project is no longer used by any system tests.

Comment on lines +15 to +16
const typescriptPath = resolve.typescript(projectRoot)
const webpackOptions = getFullWebpackOptions(file, typescriptPath)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the root of the fix. Instead of resolving typescript with our own, we get the path to it the same way we do for regular file preprocessing by finding the user's installed typescript.

@chrisbreiding chrisbreiding marked this pull request as ready for review March 3, 2023 15:56
@@ -7,6 +7,10 @@ _Released 03/14/2023 (PENDING)_

- It is now possible to control the number of connection attempts to the browser using the CYPRESS_CONNECT_RETRY_THRESHOLD Environment Variable. Learn more [here](https://docs.cypress.io/guides/references/advanced-installation#Environment-variables). Addressed in [#25848](https://github.com/cypress-io/cypress/pull/25848).

**Bugfixes:**

- Fixed an issue where using `Cypress.require()` would throw the error `Cannot find module 'typescript'`. Fixes [#25885](https://github.com/cypress-io/cypress/issues/25885).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this fixing a regression or did it never work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked with regular require, but hasn't worked with Cypress.require() since it was re-released.

@mschile mschile changed the title fix: WIP - Fix issues with Cypress.require() and TypeScript fix: Fix issues with Cypress.require() and TypeScript Mar 3, 2023

options.webpackOptions = getDefaultWebpackOptions()

addTypeScriptConfig({ filePath }, options)
if (options.typescript) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the variable we want to check is typescript.options.typescript? Is there a better name for the top variable? Or could we get options directly passed in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't quite typescript.options.typescript, but taking another look at this, I realized it had grown a little convoluted so I refactored it a bit in fa10505

@chrisbreiding chrisbreiding merged commit ed69f0b into develop Mar 8, 2023
@chrisbreiding chrisbreiding deleted the issue-25885-cypress-require-typescript branch March 8, 2023 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to use Cypress.require within cy.origin
3 participants