breaking: upgrade cy.readFile() to be a query command

cli/CHANGELOG.md

@@ -1,4 +1,12 @@
- <!-- See the ../guides/writing-the-cypress-changelog.md for details on writing the changelog. -->
+<!-- See the ../guides/writing-the-cypress-changelog.md for details on writing the changelog. -->
+## 13.0.0
+
+_Released 01/31/2023 (PENDING)_
+
+**Breaking Changes:**
+
+- The [`cy.readFile()`](/api/commands/readfile) command is now a [query command](https://on.cypress.io/retry-ability). This should not affect any tests using it; the functionality is unchanged. However, it can no longer be overwritten using [`Cypress.Commands.overwrite()`](/api/cypress-api/custom-commands#Overwrite-Existing-Commands). Addressed in [#25595](https://github.com/cypress-io/cypress/pull/25595).
+
 ## 12.4.1
 
 _Released 01/27/2023_

packages/driver/cypress/e2e/commands/assertions.cy.js

@@ -652,18 +652,21 @@ describe('src/cy/commands/assertions', () => {
         cy.get('button:first', { timeout: 500 }).should('have.class', 'does-not-have-class')
       })
 
-      it('has a pending state while retrying for commands with onFail', (done) => {
+      it('has a pending state while retrying for commands with onFail', function (done) {
         cy.on('command:retry', () => {
-          const [readFileLog, shouldLog] = cy.state('current').get('logs')
+          // Wait for the readFile response to come back from the server
+          if (this.logs.length < 2) {
+            return
+          }
+
+          const [readFileLog, shouldLog] = this.logs
 
           expect(readFileLog.get('state')).to.eq('pending')
           expect(shouldLog.get('state')).to.eq('pending')
 
           done()
         })
 
-        cy.on('fail', () => {})
-
         cy.readFile('does-not-exist.json', { timeout: 500 }).should('exist')
       })
 

packages/driver/cypress/e2e/commands/files.cy.js

@@ -14,6 +14,14 @@ describe('src/cy/commands/files', () => {
   })
 
   describe('#readFile', () => {
+    it('really works', () => {
+      cy.readFile('./cypress/fixtures/fileSpec.json').its('baseUrl').should('eq', 'http://localhost:3500')
+    })
+
+    it('works when contents are supposed to be null', () => {
+      cy.readFile('does-not-exist').should('be.null')
+    })
+
     it('triggers \'read:file\' with the right options', () => {
       Cypress.backend.resolves(okResponse)
 
@@ -80,7 +88,7 @@ describe('src/cy/commands/files', () => {
       .resolves(okResponse)
 
       cy.readFile('foo.json').then(() => {
-        expect(retries).to.eq(1)
+        expect(retries).to.eq(2)
       })
     })
 
@@ -102,18 +110,12 @@ describe('src/cy/commands/files', () => {
       })
 
       cy.readFile('foo.json').should('eq', 'quux').then(() => {
-        expect(retries).to.eq(1)
+        // Two retries: The first one triggers a backend request and throws a 'not ready' error.
+        // The second gets foobarbaz, triggering another request to the backend.
+        expect(retries).to.eq(2)
       })
     })
 
-    it('really works', () => {
-      cy.readFile('./cypress/fixtures/fileSpec.json').its('baseUrl').should('eq', 'http://localhost:3500')
-    })
-
-    it('works when contents are supposed to be null', () => {
-      cy.readFile('does-not-exist').should('be.null')
-    })
-
     describe('.log', () => {
       beforeEach(function () {
         this.logs = []
@@ -176,10 +178,6 @@ describe('src/cy/commands/files', () => {
 
         this.logs = []
 
-        cy.on('fail', () => {
-          cy.off('log:added', collectLogs)
-        })
-
         return null
       })
 
@@ -243,7 +241,7 @@ describe('src/cy/commands/files', () => {
         cy.on('fail', (err) => {
           const { fileLog } = this
 
-          assertLogLength(this.logs, 2)
+          assertLogLength(this.logs, 3)
           expect(fileLog.get('error')).to.eq(err)
           expect(fileLog.get('state')).to.eq('failed')
           expect(err.message).to.eq(stripIndent`\
@@ -388,7 +386,7 @@ describe('src/cy/commands/files', () => {
           expect(fileLog.get('error')).to.eq(err)
           expect(fileLog.get('state')).to.eq('failed')
           expect(err.message).to.eq(stripIndent`\
-            \`cy.readFile("foo")\` timed out after waiting \`10ms\`.
+            Timed out retrying after 10ms: \`cy.readFile("foo")\` timed out.
           `)
 
           expect(err.docsUrl).to.eq('https://on.cypress.io/readfile')
@@ -412,7 +410,7 @@ describe('src/cy/commands/files', () => {
           expect(fileLog.get('error')).to.eq(err)
           expect(fileLog.get('state')).to.eq('failed')
           expect(err.message).to.eq(stripIndent`\
-            \`cy.readFile("foo")\` timed out after waiting \`42ms\`.
+            Timed out retrying after 42ms: \`cy.readFile("foo")\` timed out.
           `)
 
           expect(err.docsUrl).to.eq('https://on.cypress.io/readfile')

packages/driver/cypress/e2e/e2e/origin/commands/files.cy.ts

@@ -64,11 +64,11 @@ context('cy.origin files', { browser: '!webkit' }, () => {
       })
 
       cy.shouldWithTimeout(() => {
-        const { consoleProps } = findCrossOriginLogs('readFile', logs, 'foobar.com')
+        const log = findCrossOriginLogs('readFile', logs, 'foobar.com')
 
-        expect(consoleProps.Command).to.equal('readFile')
-        expect(consoleProps['File Path']).to.include('cypress/fixtures/example.json')
-        expect(consoleProps.Contents).to.deep.equal({ example: true })
+        expect(log.consoleProps.Command).to.equal('readFile')
+        expect(log.consoleProps['File Path']).to.include('cypress/fixtures/example.json')
+        expect(log.consoleProps.Contents).to.deep.equal({ example: true })
       })
     })
 

packages/driver/src/cy/commands/files.ts

@@ -4,118 +4,132 @@ import { basename } from 'path'
 import $errUtils from '../../cypress/error_utils'
 import type { Log } from '../../cypress/log'
 
-interface InternalReadFileOptions extends Partial<Cypress.Loggable & Cypress.Timeoutable> {
-  _log?: Log
-  encoding: Cypress.Encodings
+interface ReadFileOptions extends Partial<Cypress.Loggable & Cypress.Timeoutable> {
+  encoding?: Cypress.Encodings
 }
 
 interface InternalWriteFileOptions extends Partial<Cypress.WriteFileOptions & Cypress.Timeoutable> {
   _log?: Log
 }
 
 export default (Commands, Cypress, cy, state) => {
-  Commands.addAll({
-    readFile (file, encoding, userOptions: Partial<Cypress.Loggable & Cypress.Timeoutable> = {}) {
-      if (_.isObject(encoding)) {
-        userOptions = encoding
-        encoding = undefined
+  Commands.addQuery('readFile', function readFile (file, encoding, options: ReadFileOptions = {}) {
+    if (_.isObject(encoding)) {
+      options = encoding
+      encoding = options.encoding
+    }
+
+    encoding = encoding === undefined ? 'utf8' : encoding
+
+    const timeout = options.timeout ?? Cypress.config('defaultCommandTimeout')
+
+    this.set('timeout', timeout)
+    this.set('ensureExistenceFor', 'subject')
+
+    const log = options.log !== false && Cypress.log({ message: file, timeout })
+
+    if (!file || !_.isString(file)) {
+      $errUtils.throwErrByPath('files.invalid_argument', {
+        args: { cmd: 'readFile', file },
+      })
+    }
+
+    let fileResult: any = null
+    let filePromise: Promise<void> | null = null
+    let mostRecentError = $errUtils.cypressErrByPath('files.read_timed_out', {
+      args: { file },
+    })
+
+    const createFilePromise = () => {
+      // If we already have a pending request to the backend, we'll wait
+      // for that one to resolve instead of creating a new one.
+      if (filePromise) {
+        return
       }
 
-      const options: InternalReadFileOptions = _.defaults({}, userOptions, {
+      fileResult = null
+      filePromise = Cypress.backend('read:file', file, { encoding })
+      .timeout(timeout)
+      .then((result) => {
         // https://github.com/cypress-io/cypress/issues/1558
-        // If no encoding is specified, then Cypress has historically defaulted
-        // to `utf8`, because of it's focus on text files. This is in contrast to
-        // NodeJs, which defaults to binary. We allow users to pass in `null`
-        // to restore the default node behavior.
-        encoding: encoding === undefined ? 'utf8' : encoding,
-        log: true,
-        timeout: Cypress.config('defaultCommandTimeout'),
+        // https://github.com/cypress-io/cypress/issues/20683
+        // We invoke Buffer.from() in order to transform this from an ArrayBuffer -
+        // which socket.io uses to transfer the file over the websocket - into a `Buffer`.
+        if (encoding === null && result.contents !== null) {
+          result.contents = Buffer.from(result.contents)
+        }
+
+        // Add the filename to the current command, in case we need it later (such as when storing an alias)
+        state('current').set('fileName', basename(result.filePath))
+
+        fileResult = result
       })
+      .catch((err) => {
+        if (err.name === 'TimeoutError') {
+          $errUtils.throwErrByPath('files.timed_out', {
+            args: { cmd: 'readFile', file, timeout },
+            retry: false,
+          })
+        }
 
-      const consoleProps = {}
+        // Non-ENOENT errors are not retried
+        if (err.code !== 'ENOENT') {
+          $errUtils.throwErrByPath('files.unexpected_error', {
+            args: { cmd: 'readFile', action: 'read', file, filePath: err.filePath, error: err.message },
+            errProps: { retry: false },
+          })
+        }
 
-      if (options.log) {
-        options._log = Cypress.log({
-          message: file,
-          timeout: options.timeout,
-          consoleProps () {
-            return consoleProps
-          },
+        // We have a ENOENT error - the file doesn't exist. Whether this is an error or not is deterimened
+        // by verifyUpcomingAssertions, when the command_queue receives the null file contents.
+        fileResult = { contents: null, filePath: err.filePath }
+      })
+      .catch((err) => mostRecentError = err)
+      // Pass or fail, we always clear the filePromise, so future retries know there's no
+      // live request to the server.
+      .finally(() => filePromise = null)
+    }
+
+    // When an assertion attached to this command fails, then we want to throw away the existing result
+    // and create a new promise to read a new one.
+    this.set('onFail', (err, timedOut) => {
+      if (err.type === 'existence') {
+        // file exists but it shouldn't - or - file doesn't exist but it should
+        const errPath = fileResult.contents ? 'files.existent' : 'files.nonexistent'
+        const { message, docsUrl } = $errUtils.cypressErrByPath(errPath, {
+          args: { cmd: 'readFile', file, filePath: fileResult.filePath },
         })
-      }
 
-      if (!file || !_.isString(file)) {
-        $errUtils.throwErrByPath('files.invalid_argument', {
-          onFail: options._log,
-          args: { cmd: 'readFile', file },
-        })
+        err.message = message
+        err.docsUrl = docsUrl
       }
 
-      // We clear the default timeout so we can handle
-      // the timeout ourselves
-      cy.clearTimeout()
+      createFilePromise()
+    })
 
-      const verifyAssertions = () => {
-        return Cypress.backend('read:file', file, _.pick(options, 'encoding')).timeout(options.timeout)
-        .catch((err) => {
-          if (err.name === 'TimeoutError') {
-            return $errUtils.throwErrByPath('files.timed_out', {
-              onFail: options._log,
-              args: { cmd: 'readFile', file, timeout: options.timeout },
-            })
-          }
-
-          // Non-ENOENT errors are not retried
-          if (err.code !== 'ENOENT') {
-            return $errUtils.throwErrByPath('files.unexpected_error', {
-              onFail: options._log,
-              args: { cmd: 'readFile', action: 'read', file, filePath: err.filePath, error: err.message },
-            })
-          }
-
-          return {
-            contents: null,
-            filePath: err.filePath,
-          }
-        }).then(({ filePath, contents }) => {
-          // https://github.com/cypress-io/cypress/issues/1558
-          // https://github.com/cypress-io/cypress/issues/20683
-          // We invoke Buffer.from() in order to transform this from an ArrayBuffer -
-          // which socket.io uses to transfer the file over the websocket - into a `Buffer`.
-          if (options.encoding === null && contents !== null) {
-            contents = Buffer.from(contents)
-          }
-
-          // Add the filename as a symbol, in case we need it later (such as when storing an alias)
-          state('current').set('fileName', basename(filePath))
-
-          consoleProps['File Path'] = filePath
-          consoleProps['Contents'] = contents
-
-          return cy.verifyUpcomingAssertions(contents, options, {
-            ensureExistenceFor: 'subject',
-            onFail (err) {
-              if (err.type !== 'existence') {
-                return
-              }
-
-              // file exists but it shouldn't - or - file doesn't exist but it should
-              const errPath = contents ? 'files.existent' : 'files.nonexistent'
-              const { message, docsUrl } = $errUtils.cypressErrByPath(errPath, {
-                args: { cmd: 'readFile', file, filePath },
-              })
-
-              err.message = message
-              err.docsUrl = docsUrl
-            },
-            onRetry: verifyAssertions,
-          })
-        })
+    return () => {
+      // Once we've read a file, that remains the result, unless it's cleared
+      // because of a failed assertion in `onFail` above.
+      if (fileResult) {
+        const props = {
+          'Contents': fileResult?.contents,
+          'File Path': fileResult?.filePath,
+        }
+
+        log && state('current') === this && log.set('consoleProps', () => props)
+
+        return fileResult.contents
       }
 
-      return verifyAssertions()
-    },
+      createFilePromise()
 
+      // If we don't have a result, then the promise is pending.
+      // Throw an error and wait for the promise to eventually resolve on a future retry.
+      throw mostRecentError
+    }
+  })
+
+  Commands.addAll({
     writeFile (fileName, contents, encoding, userOptions: Partial<Cypress.WriteFileOptions & Cypress.Timeoutable> = {}) {
       if (_.isObject(encoding)) {
         userOptions = encoding

packages/driver/src/cy/commands/querying/querying.ts

@@ -66,8 +66,7 @@ function getAlias (selector, log, cy) {
      */
 
     if (command.get('name') === 'intercept') {
-      // Intercept aliases are fairly similar, but `getAliasedRequests` does *not* handle indexes
-      // and we have to do it ourselves here.
+      // `getAliasedRequests` does *not* handle indexes and we have to do it ourselves here.
 
       const requests = getAliasedRequests(aliasObj.alias, cy.state)
 

packages/driver/src/cypress/error_messages.ts

@@ -520,6 +520,12 @@ export default {
         docsUrl: `https://on.cypress.io/${_.toLower(obj.cmd)}`,
       }
     },
+    read_timed_out (obj) {
+      return {
+        message: `${cmd('readFile', '"{{file}}"')} timed out.`,
+        docsUrl: `https://on.cypress.io/readfile`,
+      }
+    },
     timed_out (obj) {
       return {
         message: `${cmd('{{cmd}}', '"{{file}}"')} timed out after waiting \`{{timeout}}ms\`.`,

packages/driver/src/cypress/log.ts

@@ -148,10 +148,8 @@ const defaults = function (state: StateFunc, config, obj) {
           return {}
         }
 
-        const ret = $dom.isElement(current.get('subject')) ?
-          $dom.getElements(current.get('subject'))
-          :
-          current.get('subject')
+        const subject = current.get('subject')
+        const ret = $dom.isElement(subject) ? $dom.getElements(subject) : subject
 
         return { Yielded: ret }
       },