Skip to content

fix: Ensuring spec file presence prior to webpack-dev-server compilation #21550

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 20, 2022
Merged

fix: Ensuring spec file presence prior to webpack-dev-server compilation #21550

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
18b18a7
fix: Ensuring spec file presence prior to webpack-dev-server compilation
tbiethman May 18, 2022
fc336dd
Clean up test a bit
tbiethman May 18, 2022
4f8258c
Async beforeCompile
tbiethman May 19, 2022
0c1d3d9
Adding unit test for new behavior
tbiethman May 19, 2022
b265ee9
Moving e2e test to react specs for better project coverage
tbiethman May 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions npm/webpack-dev-server/cypress/e2e/webpack-dev-server.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,39 @@ describe('Config options', () => {
cy.contains('App.cy.jsx').click()
cy.get('.passed > .num').should('contain', 1)
})

// https://cypress-io.atlassian.net/browse/UNIFY-1697
it('filters missing spec files from loader during pre-compilation', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome test, the cy-in-cy tests for the dev-servers are so darn cool.

Copy link
Contributor

@lmiller1990 lmiller1990 May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

They are quite cool, but I wonder if a regression test would be best added in the actual npm/webpack-dev-server @tbiethman? This test is quite far away from the actual code it is related to. Not sure how practical it is to add unit/integration test inside of webpack-dev-server, though.

cy.scaffoldProject('webpack5_wds4-react')
cy.openProject('webpack5_wds4-react', ['--config-file', 'cypress-webpack.config.ts'])
cy.startAppServer('component')

cy.visitApp()

// 1. assert spec executes successfully
cy.contains('App.cy.jsx').click()
cy.get('.passed > .num').should('contain', 1)

// 2. remove file from file system
cy.withCtx(async (ctx) => {
await ctx.actions.file.removeFileInProject(`src/App.cy.jsx`)
})

// 3. assert redirect back to #/specs with alert presented
cy.contains('[data-cy="alert"]', 'Spec not found')

// 4. recreate spec, with same name as removed spec
cy.findByTestId('new-spec-button').click()
cy.findByRole('dialog').within(() => {
cy.get('input').clear().type('src/App.cy.jsx')
cy.contains('button', 'Create Spec').click()
})

cy.findByRole('dialog').within(() => {
cy.contains('button', 'Okay, run the spec').click()
})

// 5. assert recreated spec executes successfully
cy.get('.passed > .num').should('contain', 1)
})
})
13 changes: 12 additions & 1 deletion npm/webpack-dev-server/src/CypressCTWebpackPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,16 @@ export class CypressCTWebpackPlugin {
}
};

private beforeCompile = () => {
if (!this.compilation) {
return
}

// Ensure we don't try to load files that have been removed from the file system
// but have not yet been detected by the onSpecsChange handler
this.files = (this.files || []).filter((file) => fs.existsSync(file.absolute))
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure we want to use existsSync here, should we use the async pathExists from fs-extra and tapAsync?

Eg: https://github.com/cypress-io/cypress/compare/tbiethman/fix/unify-1697-ct-file-removed...lmiller1990/fix/unify-1697-ct-file-removed?expand=1

Copy link
Contributor

Choose a reason for hiding this comment

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

https://webpack.js.org/api/compiler-hooks/#beforecompile can work with tapAsync

Copy link
Contributor

Choose a reason for hiding this comment

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

Or - is the point we must do this async, so we avoid the race condition?

Copy link
Contributor Author

@tbiethman tbiethman May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We just need this to execute prior to the compile phase, which should happen regardless of whether we're sync/async here. Given that we're not actively manipulating the compile params here, we might as well use async and get whatever benefit that might have.

Your example branch was helpful, thank you. I thought about going with a Promise.all rather than running them in series. Something like:

    const foundFiles = await Promise.all(this.files.map(async (file) => {
      try {
        const exists = await fs.pathExists(file.absolute)

        return exists ? file : null
      } catch (e) {
        return null
      }
    }))

    this.files = foundFiles.filter((file) => file !== null)

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine 💯

}

/*
* After compiling, we check for errors and inform the server of them.
*/
Expand Down Expand Up @@ -96,7 +106,7 @@ export class CypressCTWebpackPlugin {
}
}

// After emitting assets, we tell the server complitation was successful
// After emitting assets, we tell the server compilation was successful
// so it can trigger a reload the AUT iframe.
private afterEmit = () => {
if (!this.compilation?.getStats().hasErrors()) {
Expand Down Expand Up @@ -152,6 +162,7 @@ export class CypressCTWebpackPlugin {
const _compiler = compiler as Compiler

this.devServerEvents.on('dev-server:specs:changed', this.onSpecsChange)
_compiler.hooks.beforeCompile.tap('CypressCTPlugin', this.beforeCompile)
_compiler.hooks.afterCompile.tap('CypressCTPlugin', this.afterCompile)
_compiler.hooks.afterEmit.tap('CypressCTPlugin', this.afterEmit)
_compiler.hooks.compilation.tap('CypressCTPlugin', (compilation) => this.addCompilationHooks(compilation as Webpack45Compilation))
Expand Down