fix: issue with compilation failures in component testing #21599

ryanthemanuel · 2022-05-23T16:14:46Z

Closes UNIFY-1437

User facing changelog

Component specs with webpack compilation errors no longer exit the server process in run mode, their results get sent to the dashboard, and the exit code is specified properly. Component specs with vite compilation errors no longer hang in run or open mode.

Additional details

Each framework handles compilation and linting errors slightly differnently. This PR focuses on compilation errors. An explanation of the current, proposed interim (on this PR), and proposed ideal states can be found in this google doc

Compilation Errors when using the Webpack Framework

The main problem with this scenario is that we currently emit a webpack failure on compilation errors and in one of the handlers, we immediately exit the server process. This doesn't allow us to return the appropriate status codes, report to the dashboard appropriately, or continue running specs that don't have compilation errors.

The interim solution to this problem is to no longer exit the process. Webpack automatically injects the compilation error into the problematic module and Cypress's generic error mechanism handles this correctly.

Longer term, we could investigate displaying a custom error for this scenario rather than the generic uncaught exception message.

Compilation Errors when using the Vite Framework

The main problem with this scenario is that esbuild errors out during the optimization process and assets are not generated.
The interim solution to this problem is to add an esbuild plugin that changes the errors to warnings. Vite will then not end up generating an asset for the bad asset and we will get a 500 error whenever the asset is loaded. This will end up getting this scenario to an acceptable state for both open and run mode, however it won't allow for the compilation error message to be displayed to the user. Instead it will be a more generic error.

In the long run, we will try and make the error more user friendly and specific. Some investigation will be needed to figure out:

How to inject the compilation error in the problematic asset in such a way that Cypress can catch it and display it to the user in a non-generic way
Can this be done with the vite-dev-server? Or do we need to try and create a static compilation in run mode? Initial investigation into this seemed to cause a stall in starting testing, so we will need to watch performance. If we do this, how do we solve open mode?

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?

…ue-with-compilation

…ue-with-compilation # Conflicts: # npm/webpack-dev-server/cypress/e2e/react.cy.ts # npm/webpack-dev-server/src/CypressCTWebpackPlugin.ts # npm/webpack-dev-server/src/devServer.ts

cypress-bot · 2022-05-23T16:14:49Z

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-05-23T16:45:04Z

Test summary

9746 • 0 • 120 • 0 • 2

Run details


Project	cypress
Status	Passed
Commit	`c9a21a7`
Started	May 25, 2022 9:23 PM
Ended	May 25, 2022 9:42 PM
Duration	19:31 💡
OS	Linux Debian - 10.11
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	e2e/origin/commands/actions.cy.ts	1
1	cy.origin actions > #consoleProps > .selectFile()
	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

This reverts commit 98b2f26.

ZachJW34

One change for the merging of the webpackConfig, the rest are minor. Tested and it works great, HUGE improvement

ZachJW34 · 2022-05-25T14:55:53Z

npm/vite-dev-server/src/resolveConfig.ts

+        incremental: true,
+        plugins: [
+          {
+            name: 'esbuild-plugin',


Can we change this to cypress-esbuild-plugin. Also, how did you debug this plugin? I tried logging and throwing errors inside the setup function but I never saw any logs inside the terminal

Done: fd175c8 (#21599)

npm/webpack-dev-server/src/makeDefaultWebpackConfig.ts

ZachJW34 · 2022-05-25T15:11:56Z

system-tests/project-fixtures/react/src/App.cy.jsx

@@ -4,5 +4,5 @@ import { App } from './App'

 it('renders hello world', () => {
  mount(<App />)
-  cy.get('h1').contains('Hello World')
+  cy.get('h1').contains('Hello World').click()


Do we need this click()?

Yes. Without it, if we are displaying an overlay the tests will pass (even though they shouldn't). I can maybe throw a comment on there to make that more clear why I'm doing it.

Comment added here: fd175c8 (#21599)

.should('be.visible') btw is more "explicit" about what you're testing. Though .click()'s actionability checks are more aggressive (and will cover more failure cases)

ZachJW34 · 2022-05-25T15:13:10Z

system-tests/lib/system-tests.ts

@@ -296,12 +296,13 @@ Bluebird.config({
  longStackTraces: true,
 })

+let cacheNodeModulesDir


Why were these changes needed?

Good catch. These were needed at one point to standardize output, but I refactored a few things and missed these. They are now removed:

fd175c8 (#21599)

BlueWinds · 2022-05-25T15:16:25Z

npm/vite-dev-server/cypress/e2e/react.cy.ts

+      cy.withCtx(async (ctx) => {
+        await ctx.actions.file.writeFileInProject(
+          `src/AppCompilationError.cy.jsx`,
+          `


Can use dedent here to indent the app code alongside the test without affecting the final file output if you like.

Updated to use dedent on the latest commit

BlueWinds · 2022-05-25T15:18:55Z

npm/vite-dev-server/src/resolveConfig.ts

+            name: 'esbuild-plugin',
+            setup (build) {
+              build.onEnd(function (result) {
+                // We don't want to completely fail the build here on errors so we treat the errors as warnings


Add link to #21599? I'd normally suggest linking the issue, but in this case it's in Jira and I prefer to link to github when possible.

Added here: fd175c8 (#21599)

BlueWinds · 2022-05-25T15:23:07Z

npm/webpack-dev-server/src/CypressCTWebpackPlugin.ts

@@ -93,42 +93,6 @@ export class CypressCTWebpackPlugin {
    callback()
  }

-  /*


Why are we removing the code to inform Cypress that there's a compilation error / success? It seems like this is an event we might want to react to in the UI in some way.

I'm thinking specifically of "compilation is done, rerun the spec" - right now we rely on webpack-dev-server's hot reload functionality, but I'd rather remove that and have Cypress do the hot reloading at some point.

See my comment below: #21599 (comment)

npm/webpack-dev-server/src/makeWebpackConfig.ts

BlueWinds · 2022-05-25T15:25:46Z

npm/webpack-dev-server/test/devServer-e2e.spec.ts

@@ -179,74 +178,6 @@ describe('#devServer', () => {
    })
  })

-  it('emits dev-server:compile:error event on error compilation', async () => {


(just another mention of the above question - if we put back the dev-server:compile:error / dev-server:compile:success events, these tests would come back as well)

So there still is a dev-server:compile:success event. It's just in a different spot, but as I was thinking about this, I like having all of the compilation hooks in one place (they were definitely spread out and confusing before). With how everything works together we don't need the error events in vite or in webpack (however, that could change as this evolves in the future).

Unified the location of the compilation hooks here:

fd175c8 (#21599)

BlueWinds · 2022-05-25T15:27:16Z

npm/webpack-dev-server/test/makeWebpackConfig.spec.ts

@@ -19,6 +19,9 @@ describe('makeWebpackConfig', () => {
        output: {
          publicPath: '/this-will-be-ignored',
        },
+        devServer: {
+          overlay: true, // This will be ignored as well


Suggested change

overlay: true, // This will be ignored as well

overlay: true, // This will be overridden by makeWebpackConfig.ts

Updated: fd175c8 (#21599)

BlueWinds · 2022-05-25T15:29:49Z

system-tests/project-fixtures/react/src/App.cy.jsx

@@ -4,5 +4,5 @@ import { App } from './App'

 it('renders hello world', () => {
  mount(<App />)
-  cy.get('h1').contains('Hello World')


Why are we clicking on hello world?

I want to ensure that the component is interactable (i.e. there's no overlay) I added a comment for this: fd175c8 (#21599)

tgriesser

Changes & rationale makes sense - good test coverage and it looks like the other review comments have been addressed, fine with me as long as tests look good 👍

tgriesser · 2022-05-25T19:02:06Z

packages/server/lib/modes/run-ct.ts

 export const run = (options: LaunchArgs, loadingPromise: Promise<void>) => {
  // TODO: make sure if we need to run this in electron by default to match e2e behavior?
  options.browser = options.browser || 'electron'
  options.runAllSpecsInSameBrowserSession = true

-  require('../plugins/dev-server').emitter.on('dev-server:compile:error', (error: Error) => {


🔥 good riddance

JessicaSachs · 2022-05-25T20:22:24Z

npm/vite-dev-server/cypress/support/e2e.ts

@@ -17,4 +17,4 @@
 import '@packages/frontend-shared/cypress/e2e/support/e2eSupport'

 // Alternatively you can use CommonJS syntax:
-// require('./commands')
+require('./commands')


I don't like mixing these. Why would you require instead of import? Not gonna block, but we should stick to one way of importing. require is nonstandard in the browser.

I can update this. That was my bad. I had just uncommented what was commented.

Updated here: dd566a0 (#21599)

JessicaSachs

Concerned about magicHtml usage. What's the deal with that?

JessicaSachs · 2022-05-25T20:26:32Z

npm/webpack-dev-server/test/makeWebpackConfig.spec.ts

+          publicPath: '/this-will-be-ignored',
+        },
+        devServer: {
+          magicHtml: true,


This was added in Webpack 4.1 only. Will we break user config for Webpack 4? Is there a sys test for Webpack 4.0?

@JessicaSachs this is a unit test. I'm just verifying that user defined config will be kept (other than the ones we care about)

Yeah I saw that was just like "why this flag?". Cool.

JessicaSachs · 2022-05-25T20:27:20Z

packages/server/lib/modes/run-ct.ts

 export const run = (options: LaunchArgs, loadingPromise: Promise<void>) => {
  // TODO: make sure if we need to run this in electron by default to match e2e behavior?
  options.browser = options.browser || 'electron'
  options.runAllSpecsInSameBrowserSession = true

-  require('../plugins/dev-server').emitter.on('dev-server:compile:error', (error: Error) => {


…pack * develop: fix: UNIFY-1408, warnings should be nested appropriately & clearable (#21630) chore: fix unit-tests-release job (#21652) chore(deps): update dependency eventsource to v2 [security] (#21639) fix: Add hover states for test titles in reporter (#21635) docs(CONTRIBUTING): Fix link to "good first issue" for newcomers (#21614) chore: compare `cy.screenshot` images in percy (#21598) fix: switching from ct to e2e (non-configured) does not go through setup (#21607) fix: issue with compilation failures in component testing (#21599) test: fix flaky launchpad test (#21637) docs: remove gitter link in contributing guide. (#21592) fix: order projects by most recently opened (#21589) fix: prevent crash on runs visit when offline (#21618) fix: pass family parameter to connect method (#21545) chore: clean up `debug` statements in preparation for 10.0 release, add `debug` docs (#21621) chore: add regression test for ts detection (#21578)

ryanthemanuel added 17 commits May 18, 2022 19:31

fix: issue with compilation failures in component testing

ab5dba7

add tests

fa0bdde

fix tests

2ea4c55

Refactor tests

d6250b3

Fix tests

8bb8072

Refactor tests

26640ac

Fix tests

c802e3e

Fix paths for dependencies in system tests

6b17c82

Fix tests

b83d0d6

Merge remote-tracking branch 'origin/10.0-release' into ryanm/fix/iss…

9197a34

…ue-with-compilation

Fix tests

4968f0e

Fix tests

865d69f

Fix tests

4f1c5b6

test fix for initial esbuild failures

5a19489

Fix tests

0d90402

Add back ESLintPlugin

67ca85c

Merge remote-tracking branch 'origin/10.0-release' into ryanm/fix/iss…

4da5a3e

…ue-with-compilation # Conflicts: # npm/webpack-dev-server/cypress/e2e/react.cy.ts # npm/webpack-dev-server/src/CypressCTWebpackPlugin.ts # npm/webpack-dev-server/src/devServer.ts

Add comments around our special esbuild handling logic in vite

148442f

ryanthemanuel added 4 commits May 23, 2022 12:33

Code cleanup and additional test scenario

8acd0e8

Merge branch '10.0-release' into ryanm/fix/issue-with-compilation

04e928d

Add config syntax error tests

6dcc16b

Improve tests

fbf3cb6

ryanthemanuel marked this pull request as ready for review May 24, 2022 01:31

ryanthemanuel requested review from a team as code owners May 24, 2022 01:31

ryanthemanuel requested review from jennifer-shehane and tgriesser and removed request for a team May 24, 2022 01:31

ryanthemanuel requested review from tbiethman and removed request for a team and jennifer-shehane May 24, 2022 13:54

ryanthemanuel added 6 commits May 24, 2022 08:54

Merge branch '10.0-release' into ryanm/fix/issue-with-compilation

5b8dec6

Update comment and remove unused variable

2ba02b0

Remove unneeded hook in webpack dev server

98b2f26

Disable dev server overlay

3ff4de3

Merge branch '10.0-release' into ryanm/fix/issue-with-compilation

5f14540

Revert "Remove unneeded hook in webpack dev server"

741f89f

This reverts commit 98b2f26.

ZachJW34 changed the base branch from 10.0-release to develop May 25, 2022 14:12

Merge branch 'develop' into ryanm/fix/issue-with-compilation

358a566

ryanthemanuel requested a review from BlueWinds May 25, 2022 14:40

ZachJW34 suggested changes May 25, 2022

View reviewed changes

BlueWinds reviewed May 25, 2022

View reviewed changes

ryanthemanuel added 3 commits May 25, 2022 11:49

PR comments

fd175c8

Accidental removal

4f6d116

Fix dedent

39d60b8

ryanthemanuel requested review from BlueWinds and ZachJW34 May 25, 2022 18:53

BlueWinds approved these changes May 25, 2022

View reviewed changes

ZachJW34 approved these changes May 25, 2022

View reviewed changes

tgriesser approved these changes May 25, 2022

View reviewed changes

JessicaSachs reviewed May 25, 2022

View reviewed changes

PR comments

dd566a0

JessicaSachs reviewed May 25, 2022

View reviewed changes

Merge branch 'develop' into ryanm/fix/issue-with-compilation

c9a21a7

ryanthemanuel merged commit f2bce02 into develop May 25, 2022

ryanthemanuel deleted the ryanm/fix/issue-with-compilation branch May 25, 2022 23:02

ZachJW34 mentioned this pull request Aug 9, 2022

Projects for some frameworks don't surface compilation errors/warnings #23219

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: issue with compilation failures in component testing #21599

fix: issue with compilation failures in component testing #21599

ryanthemanuel commented May 23, 2022 •

edited

Loading

cypress-bot bot commented May 23, 2022

cypress bot commented May 23, 2022 •

edited

Loading

ZachJW34 left a comment

ZachJW34 May 25, 2022

ryanthemanuel May 25, 2022

ZachJW34 May 25, 2022

ryanthemanuel May 25, 2022

ryanthemanuel May 25, 2022

JessicaSachs May 25, 2022 •

edited

Loading

ZachJW34 May 25, 2022

ryanthemanuel May 25, 2022

BlueWinds May 25, 2022

ryanthemanuel May 25, 2022

BlueWinds May 25, 2022

ryanthemanuel May 25, 2022

BlueWinds May 25, 2022

ryanthemanuel May 25, 2022

BlueWinds May 25, 2022

ryanthemanuel May 25, 2022

BlueWinds May 25, 2022

ryanthemanuel May 25, 2022

BlueWinds May 25, 2022

ryanthemanuel May 25, 2022

tgriesser left a comment

tgriesser May 25, 2022

JessicaSachs May 25, 2022

JessicaSachs May 25, 2022 •

edited

Loading

ryanthemanuel May 25, 2022 •

edited

Loading

ryanthemanuel May 25, 2022

JessicaSachs left a comment

JessicaSachs May 25, 2022

ryanthemanuel May 25, 2022 •

edited

Loading

JessicaSachs May 25, 2022

JessicaSachs May 25, 2022

	overlay: true, // This will be ignored as well
	overlay: true, // This will be overridden by makeWebpackConfig.ts

fix: issue with compilation failures in component testing #21599

fix: issue with compilation failures in component testing #21599

Conversation

ryanthemanuel commented May 23, 2022 • edited Loading

User facing changelog

Additional details

Compilation Errors when using the Webpack Framework

Compilation Errors when using the Vite Framework

How has the user experience changed?

PR Tasks

cypress-bot bot commented May 23, 2022

cypress bot commented May 23, 2022 • edited Loading

Test summary

Run details

Flakiness

ZachJW34 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

JessicaSachs May 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

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

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

JessicaSachs May 25, 2022 • edited Loading

Choose a reason for hiding this comment

ryanthemanuel May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JessicaSachs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanthemanuel May 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

ryanthemanuel commented May 23, 2022 •

edited

Loading

cypress bot commented May 23, 2022 •

edited

Loading

JessicaSachs May 25, 2022 •

edited

Loading

JessicaSachs May 25, 2022 •

edited

Loading

ryanthemanuel May 25, 2022 •

edited

Loading

ryanthemanuel May 25, 2022 •

edited

Loading