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

[BlueWinds]

Additional details

This PR updates cy.readFile() to be a query command. It is a breaking change because users will no longer be able to overwrite readFile with Cypress.Commands.overwrite(). No other impact on user's tests is anticipated.

cy.readFile() already matched the behavior of other queries, by reading the file from disk until upcoming assertions pass. This update therefore is not a behavior change - it simply brings the readFile code execution path in line with other queries which do the same thing.

// Rereads file from disk until assertion passes
cy.readFile('foo.json').should('eql', { user: 'bar', id: 123 })

// Requeries element from DOM until assertion passes
cy.get('#div').should('have.class', 'active')

// Does not reread file from disk
cy.fixture('foo.json').should('eql', { user: 'bar', id: 123 })

Despite there being no user-facing changes, this update is a prerequisite for #25296. In that pull request, assertions will no longer "belong" to previous commands - they execute entirely independently. That means that the only way for a command to update the subject in response to failed assertions is to be a query - .readFile() can no longer be a special case of a "query-like command that isn't a query."

When reviewing / aiming to understand the changes, suggest starting with this comment: #25595 (comment).

Steps to test

No behavior has changed, and tests required no updates (see comments on each individual change for why it is code-clarity related and not functional).

How has the user experience changed?

Previously, if readFile timed out, users would get the message

`cy.readFile(foo.txt)` timed out retrying after 4000ms.

They now instead get the message

Timed out retrying after 4000ms: `cy.readFile(foo.txt)` timed out.

PR Tasks

• Have tests been added/updated?
• Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
• Has a PR for user-facing changes been opened in cypress-documentation? .readFile() is now a query cypress-documentation#5017
• [n/a] Have API changes been updated in the type definitions?

[cypress]

6 flaky tests on run #43697 ↗︎

0

4284

879

0

6

Details:

Merge branch 'release/13.0.0' into issue-25134-readfile-as-query
Project: cypress	Commit: 96e75d6d7b
Status: Passed	Duration: 15:49 💡
Started: Feb 1, 2023 8:54 PM	Ended: Feb 1, 2023 9:10 PM

  commands/net_stubbing.cy.ts • 3 flaky tests • 5x-driver-webkit

View Output Video

Test
network stubbing > intercepting request > can delay and throttle a StaticResponse
... > with `times` > only uses each handler N times
... > stops waiting when an xhr request is canceled

  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-webkit

View Output Video

Test
... > correctly returns currentRetry
... > correctly returns currentRetry
... > correctly returns currentRetry

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

[BlueWinds]

Moved the simplest tests higher up in the file. It felt off that we started with detailed assertions about the interaction with the backend before just using the command plainly.

[BlueWinds]

When mocked, the new version always retries one more time than the non-query version. This occurs because the first invocation triggers the promise which reads the file from disk and throws an error; it is always asynchronous, even when mocked.

[BlueWinds]

Having an on('fail') handler in the beforeEach() block makes it hard to debug actual failures, since if you comment out a test's onFail block, the test will suddenly pass.

The only change this makes is that in some tests we see three logs rather than two, because "the stub has been called" log is included (where previously it would have been ignored because of this handler-removal).

[BlueWinds]

No functional change here. While working on these changes, I found it easier to read test failures when they were of the form "unable to read consoleProps from null" rather than "unable to destructure intermediate object" (the actual errors are significantly more verbose and harder to read than the above).

[BlueWinds]

The way this now works is basically a state machine, with three pieces of state:

1. The contents of the file read from disk (can be null or have a value)
2. The currently active promise, waiting for a response from the server (can be null or be a promise)
3. The most recent error (always has a value)

The state can be updated in three different places.

1. When the query function executes (return () => {), if we have a fileResult, return it. Otherwise, create a new filePromise if we don't already have one (createFilePromise()) and throw mostRecentError.
2. When filePromise resolves (.then((result) => {), we set the contents to fileResult, and clear filePromise.
3. When filePromise rejects:
   a. If it rejects ((.catch((err) => {)) with "file doesn't exist", we set fileResult to "no file exists" and clear filePromise.
   b. Otherwise, we set the error to mostRecentError, and clear filePromise.
4. If an upcoming assertion fails, we clear fileResult.

The end result is that we query the disk for a file, and throw a "timed out" error until we get back a result (including "no file exists"); if the server responds with an unexpected error, we start throwing that instead of "timed out" and retry.

If it succeeds, we start returning that file as the result.

If an upcoming assertion fails (including the implicit "file should exist" assertion), we clear the result and head back into the retry loop.

[BlueWinds]

Similar to the other test changes, this one relied on the state after the first retry; we no longer wait for a server response before "retrying" - so the test needs to be more patient.

[chrisbreiding]

Looks good! Just one little suggestion for the error message you can take or leave. I had originally thought a command like readFile couldn't be made into a query since queries have to be synchronous, so it's interesting to see how you've made it work.

Should this be considered a breaking change? Since readFile is now a query, any users overwriting it as a command will no longer be able to do so.

[BlueWinds]

Looks good! Just one little suggestion for the error message you can take or leave. I had originally thought a command like readFile couldn't be made into a query since queries have to be synchronous, so it's interesting to see how you've made it work.

That was definitely my initial thought as well; I spent some time while working on this PR wondering if I'd dropped the ball a bit with queries and they could be async after all, but I think the answer remains the same - it's a hard limitation in the design that can be hacked around, but not changed.

I've been considering if it might be possible to provide a utility function to do this in a more generic way - the signature would be something like

createQueryCallback(generatePromise, initialError)

where you call it with a function that returns a promise and "the error if we don't have another error" and it returns a query function. It's a bit more complicated to make it generic than it seems, which is why I didn't end up going down that route in this pull request, but I think the possibility is there.

Should this be considered a breaking change? Since readFile is now a query, any users overwriting it as a command will no longer be able to do so.

Good call, I forgot about that side effect. Will update changelog.

[emilyrohrbough]

@BlueWinds Can you create a release-13.0.0 branch to base this off of?

The base branch was changed.

[BlueWinds]

@BlueWinds Can you create a release-13.0.0 branch to base this off of?

Done.

Also created the docs PR - cypress-io/cypress-documentation#5017.

[chrisbreiding]

Would these changes be useful in develop? Maybe they could be cherry-picked into another PR against develop?

[BlueWinds]

@emilyrohrbough , thoughts?

[emilyrohrbough]

I have a branch in progress for this.

[jennifer-shehane]

Released in Cypress 13.0.0.