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: retry on EMFILE always, lint sync FS calls #22175

Merged
merged 18 commits into from Jun 16, 2022
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 29 additions & 0 deletions .eslintrc.js
Expand Up @@ -5,6 +5,7 @@ const { specifiedRules } = require('graphql')
const graphqlOpts = {
env: 'literal',
tagName: 'gql',
// eslint-disable-next-line no-restricted-syntax
schemaString: fs.readFileSync(
path.join(__dirname, 'packages/graphql/schemas/schema.graphql'),
'utf8',
Expand Down Expand Up @@ -33,6 +34,24 @@ module.exports = {
'plugin:@cypress/dev/tests',
],
parser: '@typescript-eslint/parser',
overrides: [
{
files: [
// ignore in tests and scripts
'**/scripts/**',
'**/test/**',
'**/system-tests/**',
'packages/{app,driver,frontend-shared,launchpad}/cypress/**',
'*.test.ts',
// ignore in packages that don't run in the Cypress process
'npm/create-cypress-tests/**',
],
rules: {
'no-restricted-properties': 'off',
'no-restricted-syntax': 'off',
},
},
],
rules: {
'no-duplicate-imports': 'off',
'import/no-duplicates': 'off',
Expand All @@ -55,6 +74,16 @@ module.exports = {
message: 'os.userInfo() will throw when there is not an `/etc/passwd` entry for the current user (like when running with --user 12345 in Docker). Do not use it unless you catch any potential errors.',
},
],
'no-restricted-syntax': [
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, hope we don't need to edit this in the near future, did not realize you could do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I pasted a URL there that helps you debug esquery, but even still this took me a bit to figure out.

// esquery tool: https://estools.github.io/esquery/
'error',
{
// match sync FS methods except for `existsSync`
// examples: fse.readFileSync, fs.readFileSync, this.ctx.fs.readFileSync...
selector: `MemberExpression[object.name='fs'][property.name=/^[A-z]+Sync$/]:not(MemberExpression[property.name='existsSync']), MemberExpression[property.name=/^[A-z]+Sync$/]:not(MemberExpression[property.name='existsSync']):has(MemberExpression[property.name='fs'])`,
message: 'Synchronous fs calls should not be used in Cypress. Use an async API instead.',
},
],
'graphql/capitalized-type-name': ['warn', graphqlOpts],
'graphql/no-deprecated-fields': ['error', graphqlOpts],
'graphql/template-strings': ['error', { ...graphqlOpts, validators }],
Expand Down
1 change: 1 addition & 0 deletions npm/eslint-plugin-dev/lib/custom-rules/index.js
Expand Up @@ -2,6 +2,7 @@ const fs = require('fs')
const path = require('path')

module.exports =
// eslint-disable-next-line no-restricted-syntax
Object.assign({}, ...fs.readdirSync(__dirname)
.filter((filename) => filename.endsWith('.js') && filename !== 'index.js')
.map((filename) => ({ [filename.replace(/\.js$/u, '')]: require(path.resolve(__dirname, filename)) })))
2 changes: 1 addition & 1 deletion npm/vite-dev-server/cypress/e2e/vite-dev-server.cy.ts
Expand Up @@ -38,7 +38,7 @@ describe('Config options', () => {
cy.startAppServer('component')

cy.withCtx(async (ctx, { specWithWhitespace }) => {
ctx.actions.file.writeFileInProject(
await ctx.actions.file.writeFileInProject(
ctx.path.join('src', specWithWhitespace),
await ctx.file.readFileInProject(ctx.path.join('src', 'App.cy.jsx')),
)
Expand Down
2 changes: 2 additions & 0 deletions npm/vite-dev-server/src/plugins/cypress.ts
Expand Up @@ -38,6 +38,8 @@ export const Cypress = (
const indexHtmlFile = options.cypressConfig.indexHtmlFile

let specsPathsSet = getSpecsPathsSet(specs)
// TODO: use async fs methods here
// eslint-disable-next-line no-restricted-syntax
let loader = fs.readFileSync(INIT_FILEPATH, 'utf8')
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this "do not use sync FS calls" also need to be applied in npm, or just things we know are shipping in the binary (and never published separately on npm). It's possible a user is consuming these outside of Cypress (however unlikely) and they may not have access to wrapped fs module with async extensions.

Seems safest to either us things we know exist in the Node.js standard lib, or have these depend on fs-extra, or use fs.promises (didn't know about this!) to making async versions of the functions we need in each npm module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd use fs.promises here if I was gonna change this. I started to, but then stopped because it actually requires a user-facing (I think?) type change to make this function async.

Since this is most likely to run in the plugins process, it should be fine to just use fs.promises, since we already run gracefulFs.gracefulify(fs) in the child process:

Copy link
Contributor

Choose a reason for hiding this comment

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

Not too sure this would require a user facing change (this function is internal to the Vite dev server) but sure, happy to leave it like this for now. If we do change it fs.promises seems good.


devServerEvents.on('dev-server:specs:changed', (specs: Spec[]) => {
Expand Down
4 changes: 3 additions & 1 deletion npm/webpack-dev-server/src/CypressCTWebpackPlugin.ts
Expand Up @@ -99,13 +99,15 @@ export class CypressCTWebpackPlugin {
* has been "updated on disk", causing a recompliation (and pulling the new specs in as
* dependencies).
*/
private onSpecsChange = (specs: Cypress.Cypress['spec'][]) => {
private onSpecsChange = async (specs: Cypress.Cypress['spec'][]) => {
if (!this.compilation || _.isEqual(specs, this.files)) {
return
}

this.files = specs
const inputFileSystem = this.compilation.inputFileSystem
// TODO: don't use a sync fs method here
// eslint-disable-next-line no-restricted-syntax
const utimesSync: UtimesSync = inputFileSystem.fileSystem.utimesSync ?? fs.utimesSync

utimesSync(path.resolve(__dirname, 'browser.js'), new Date(), new Date())
Expand Down
12 changes: 6 additions & 6 deletions npm/webpack-dev-server/src/helpers/nextHandler.ts
Expand Up @@ -69,7 +69,7 @@ async function loadWebpackConfig (devServerConfig: WebpackDevServerConfig): Prom
buildId: `@cypress/react-${Math.random().toString()}`,
config: nextConfig,
dev: true,
pagesDir: findPagesDir(devServerConfig.cypressConfig.projectRoot),
pagesDir: await findPagesDir(devServerConfig.cypressConfig.projectRoot),
entrypoints: {},
rewrites: { fallback: [], afterFiles: [], beforeFiles: [] },
...runWebpackSpan,
Expand Down Expand Up @@ -106,9 +106,9 @@ function checkSWC (
return false
}

const existsSync = (file: string) => {
const exists = async (file: string) => {
try {
fs.accessSync(file, fs.constants.F_OK)
await fs.promises.access(file, fs.constants.F_OK)

return true
} catch (_) {
Expand All @@ -121,16 +121,16 @@ const existsSync = (file: string) => {
* `${projectRoot}/pages` or `${projectRoot}/src/pages`.
* If neither is found, return projectRoot
*/
function findPagesDir (projectRoot: string) {
async function findPagesDir (projectRoot: string) {
// prioritize ./pages over ./src/pages
let pagesDir = path.join(projectRoot, 'pages')

if (existsSync(pagesDir)) {
if (await exists(pagesDir)) {
return pagesDir
}

pagesDir = path.join(projectRoot, 'src', 'pages')
if (existsSync(pagesDir)) {
if (await exists(pagesDir)) {
return pagesDir
}

Expand Down
1 change: 1 addition & 0 deletions npm/webpack-dev-server/src/measureWebpackPerformance.ts
@@ -1,3 +1,4 @@
/* eslint-disable no-restricted-syntax */
/* eslint-disable no-console */
import path from 'path'
import fs from 'fs'
Expand Down
2 changes: 1 addition & 1 deletion npm/webpack-preprocessor/package.json
Expand Up @@ -11,7 +11,7 @@
"secure": "nsp check",
"semantic-release": "semantic-release",
"size": "npm pack --dry",
"test": "node ./test-webpack-5.js",
"test": "node ./scripts/test-webpack-5.js",
"test-debug": "node --inspect --debug-brk ./node_modules/.bin/_mocha",
"test-e2e": "mocha test/e2e/*.spec.*",
"test-unit": "mocha test/unit/*.spec.*",
Expand Down
@@ -1,7 +1,10 @@
const execa = require('execa')
const pkg = require('./package.json')
const path = require('path')
const pkg = require('../package.json')
const fs = require('fs')

const pkgJsonPath = path.join(__dirname, '..', 'package.json')

/**
* This file installs Webpack 5 and runs the unit and e2e tests for the preprocessor.
* We read package.json, update the webpack version, then re-run yarn install.
Expand All @@ -17,7 +20,7 @@ const main = async () => {
const install = () => execa('yarn', ['install', '--ignore-scripts'], { stdio: 'inherit' })

const resetPkg = async () => {
fs.writeFileSync('package.json', originalPkg, 'utf8')
fs.writeFileSync(pkgJsonPath, originalPkg, 'utf8')
await install()
}

Expand All @@ -38,7 +41,7 @@ const main = async () => {
delete pkg.devDependencies['webpack']
// eslint-disable-next-line no-console
console.log('[@cypress/webpack-preprocessor]: updating package.json...')
fs.writeFileSync('package.json', JSON.stringify(pkg, null, 2))
fs.writeFileSync(pkgJsonPath, JSON.stringify(pkg, null, 2))

// eslint-disable-next-line no-console
console.log('[@cypress/webpack-preprocessor]: install dependencies...')
Expand Down
4 changes: 2 additions & 2 deletions packages/app/cypress/e2e/cypress-in-cypress-component.cy.ts
Expand Up @@ -208,8 +208,8 @@ describe('Cypress In Cypress CT', { viewportWidth: 1500, defaultCommandTimeout:
}).invoke('connected').should('be.true')
})

cy.withCtx((ctx, o) => {
ctx.actions.file.writeFileInProject(o.path, `
cy.withCtx(async (ctx, o) => {
await ctx.actions.file.writeFileInProject(o.path, `
import React from 'react'
import { mount } from '@cypress/react'

Expand Down
4 changes: 2 additions & 2 deletions packages/app/cypress/e2e/cypress-in-cypress-e2e.cy.ts
Expand Up @@ -259,8 +259,8 @@ describe('Cypress In Cypress E2E', { viewportWidth: 1500, defaultCommandTimeout:
}).invoke('connected').should('be.true')
})

cy.withCtx((ctx, o) => {
ctx.actions.file.writeFileInProject(o.path, `
cy.withCtx(async (ctx, o) => {
await ctx.actions.file.writeFileInProject(o.path, `
describe('Dom Content', () => {
it('renders the new test content', () => {
cy.visit('cypress/e2e/dom-content.html')
Expand Down
24 changes: 12 additions & 12 deletions packages/app/cypress/e2e/cypress-in-cypress.cy.ts
Expand Up @@ -228,13 +228,13 @@ describe('Cypress in Cypress', { viewportWidth: 1500, defaultCommandTimeout: 100
startAtSpecsPage('e2e')
cy.get('[data-cy="spec-item"]')

cy.withCtx((ctx, o) => {
cy.withCtx(async (ctx, o) => {
ctx.coreData.app.browserStatus = 'open'

let config = ctx.actions.file.readFileInProject('cypress.config.js')
let config = await ctx.actions.file.readFileInProject('cypress.config.js')

config = config.replace(`e2e: {`, `e2e: {\n chromeWebSecurity: false,\n`)
ctx.actions.file.writeFileInProject('cypress.config.js', config)
await ctx.actions.file.writeFileInProject('cypress.config.js', config)

o.sinon.stub(ctx.actions.browser, 'closeBrowser')
o.sinon.stub(ctx.actions.browser, 'relaunchBrowser')
Expand All @@ -252,24 +252,24 @@ describe('Cypress in Cypress', { viewportWidth: 1500, defaultCommandTimeout: 100
it('restarts browser if there is a before:browser:launch task and there is a change on the config', () => {
startAtSpecsPage('e2e')

cy.withCtx((ctx, o) => {
cy.withCtx(async (ctx, o) => {
ctx.coreData.app.browserStatus = 'open'

let config = ctx.actions.file.readFileInProject('cypress.config.js')
let config = await ctx.actions.file.readFileInProject('cypress.config.js')

config = config.replace(`e2e: {`, `e2e: {\n setupNodeEvents(on) {\n on('before:browser:launch', () => {})\n},\n`)
ctx.actions.file.writeFileInProject('cypress.config.js', config)
await ctx.actions.file.writeFileInProject('cypress.config.js', config)
})

cy.get('[data-cy="spec-item"]')

cy.withCtx((ctx, o) => {
cy.withCtx(async (ctx, o) => {
ctx.coreData.app.browserStatus = 'open'

let config = ctx.actions.file.readFileInProject('cypress.config.js')
let config = await ctx.actions.file.readFileInProject('cypress.config.js')

config = config.replace(`e2e: {`, `e2e: {\n viewportHeight: 600,\n`)
ctx.actions.file.writeFileInProject('cypress.config.js', config)
await ctx.actions.file.writeFileInProject('cypress.config.js', config)

o.sinon.stub(ctx.actions.browser, 'closeBrowser')
o.sinon.stub(ctx.actions.browser, 'relaunchBrowser')
Expand All @@ -288,14 +288,14 @@ describe('Cypress in Cypress', { viewportWidth: 1500, defaultCommandTimeout: 100
startAtSpecsPage('e2e')
cy.get('[data-cy="spec-item"]')

cy.withCtx((ctx, o) => {
cy.withCtx(async (ctx, o) => {
ctx.coreData.app.browserStatus = 'open'
o.sinon.stub(ctx.actions.project, 'initializeActiveProject')

let config = ctx.actions.file.readFileInProject('cypress.config.js')
let config = await ctx.actions.file.readFileInProject('cypress.config.js')

config = config.replace(`{`, `{\n watchForFileChanges: false,\n`)
ctx.actions.file.writeFileInProject('cypress.config.js', config)
await ctx.actions.file.writeFileInProject('cypress.config.js', config)
})

cy.get('[data-cy="loading-spinner"]').should('be.visible')
Expand Down
4 changes: 2 additions & 2 deletions packages/app/cypress/e2e/specs.cy.ts
Expand Up @@ -423,7 +423,7 @@ describe('App: Specs', () => {

it('generates spec with file name that does not contain a known spec extension', () => {
cy.withCtx(async (ctx) => {
let config = ctx.actions.file.readFileInProject('cypress.config.js')
let config = await ctx.actions.file.readFileInProject('cypress.config.js')

config = config.replace(
`specPattern: 'src/**/*.{cy,spec}.{js,jsx}'`,
Expand Down Expand Up @@ -700,7 +700,7 @@ describe('App: Specs', () => {

it('generates spec with file name that does not contain a known spec extension', () => {
cy.withCtx(async (ctx) => {
let config = ctx.actions.file.readFileInProject('cypress.config.js')
let config = await ctx.actions.file.readFileInProject('cypress.config.js')

config = config.replace(
`specPattern: 'src/specs-folder/*.cy.{js,jsx}'`,
Expand Down
@@ -1,18 +1,18 @@
function updateProjectIdInCypressConfig (value: string) {
return cy.withCtx((ctx, o) => {
let config = ctx.actions.file.readFileInProject('cypress.config.js')
return cy.withCtx(async (ctx, o) => {
let config = await ctx.actions.file.readFileInProject('cypress.config.js')

config = config.replace(`projectId: 'abc123'`, `projectId: '${o.value}'`)
ctx.actions.file.writeFileInProject('cypress.config.js', config)
await ctx.actions.file.writeFileInProject('cypress.config.js', config)
}, { value })
}

function updateViewportHeightInCypressConfig (value: number) {
return cy.withCtx((ctx, o) => {
let config = ctx.actions.file.readFileInProject('cypress.config.js')
return cy.withCtx(async (ctx, o) => {
let config = await ctx.actions.file.readFileInProject('cypress.config.js')

config = config.replace(`e2e: {`, `e2e: {\n viewportHeight: ${o.value},\n`)
ctx.actions.file.writeFileInProject('cypress.config.js', config)
await ctx.actions.file.writeFileInProject('cypress.config.js', config)
}, { value })
}

Expand Down
Expand Up @@ -18,8 +18,8 @@ describe('specChange subscription', () => {
.should('contain', 'dom-content.spec.js')
.should('contain', 'dom-list.spec.js')

cy.withCtx((ctx, o) => {
ctx.actions.file.writeFileInProject(o.path, '')
cy.withCtx(async (ctx, o) => {
await ctx.actions.file.writeFileInProject(o.path, '')
}, { path: getPathForPlatform('cypress/e2e/new-file.spec.js') })

cy.get('[data-cy="spec-item-link"]')
Expand Down
16 changes: 8 additions & 8 deletions packages/data-context/src/actions/FileActions.ts
Expand Up @@ -8,29 +8,29 @@ import assert from 'assert'
export class FileActions {
constructor (private ctx: DataContext) {}

readFileInProject (relativePath: string): string {
async readFileInProject (relativePath: string): Promise<string> {
if (!this.ctx.currentProject) {
throw new Error(`Cannot write file in project without active project`)
}

const filePath = path.join(this.ctx.currentProject, relativePath)

this.ctx.fs.ensureDirSync(path.dirname(filePath))
await this.ctx.fs.ensureDir(path.dirname(filePath))

return this.ctx.fs.readFileSync(filePath, 'utf-8')
return this.ctx.fs.readFile(filePath, 'utf-8')
}

writeFileInProject (relativePath: string, data: any) {
async writeFileInProject (relativePath: string, data: any) {
if (!this.ctx.currentProject) {
throw new Error(`Cannot write file in project without active project`)
}

const filePath = path.join(this.ctx.currentProject, relativePath)

this.ctx.fs.ensureDirSync(path.dirname(filePath))
await this.ctx.fs.ensureDir(path.dirname(filePath))

// Typically used in e2e tests, simpler than forcing async
this.ctx.fs.writeFileSync(
await this.ctx.fs.writeFile(
filePath,
data,
)
Expand All @@ -42,7 +42,7 @@ export class FileActions {
}

// Typically used in e2e tests, simpler than forcing async
this.ctx.fs.removeSync(path.join(this.ctx.currentProject, relativePath))
await this.ctx.fs.remove(path.join(this.ctx.currentProject, relativePath))
}

async moveFileInProject (relativePath: string, toRelativePath: string) {
Expand All @@ -51,7 +51,7 @@ export class FileActions {
}

// Typically used in e2e tests, simpler than forcing async
this.ctx.fs.moveSync(
await this.ctx.fs.move(
path.join(this.ctx.currentProject, relativePath),
path.join(this.ctx.currentProject, toRelativePath),
)
Expand Down