feat: Allow explicit null encoding for readFile/writeFile #18534

BlueWinds · 2021-10-18T19:18:27Z

Closes reading binary file from fixture differs from drop event file #1558

User facing changelog

When null is passed as the encoding to cy.readFile(), the file is treated as binary and read as a Buffer. Similarly, null passed as the encoding to cy.writeFile() allows direct writing of buffers.

If encoding is unspecified, the default remains 'utf8', matching current behavior.

Additional details

This should significantly ease the effort of working with binary files from within cypress tests.

As an example:

    cy.writeFile('foo.bin', Buffer.from([0, 0, 12, 54. 255]), { encoding: null })
    cy.readFile('foo.bin', { encoding: null }).should('eql', Buffer.from([0, 0, 12, 54, 255]))

How has the user experience changed?

(this screenshot does not exactly match the above example code)

PR Tasks

Have tests been added/updated?
Has the original issue or this PR been tagged with a release in ZenHub?
Has a PR for user-facing changes been opened in cypress-documentation? Add explicit null as an option for encoding cypress-documentation#4172

cypress-bot · 2021-10-18T19:18:30Z

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 · 2021-10-18T19:27:36Z

Test summary

4212 • 0 • 50 • 2 • 0

Run details


Project	cypress
Status	Passed
Commit	`5905fdf`
Started	Oct 25, 2021 8:23 PM
Ended	Oct 25, 2021 8:33 PM
Duration	09:43 💡
OS	Linux Debian - 10.9
Browser	Chrome 95

View run in Cypress Dashboard ➡️

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

BlueWinds · 2021-10-18T19:51:53Z

I haven't written docs for these changes yet, wanted to get some eyes on it and see if the approach was right before first. @sainthkh, @jennifer-shehane suggested you might be interested in taking a look at these changes.

sainthkh

Hello!

The code is good and there are only a few minor changes.

1. User facing changelog

When { encoding: null } is passed to cy.readFile(), the file is treated as binary and read as a Buffer. Similarly, { encoding: null } passed to cy.writeFile() allows direct writing of buffers.

Although { encoding: value } works, it is recommended to pass encoding as the second arg. So, it might be better to change this like:

When null is passed to encoding of cy.readFile(), the file is treated as binary and read as a Buffer. Similarly, null passed to encoding of cy.writeFile() allows direct writing of buffers.

2. Add null to Encodings types.

cypress/cli/types/cypress.d.ts

Line 5675 in a045e4f

    
           type Encodings = 'ascii' | 'base64' | 'binary' | 'hex' | 'latin1' | 'utf8' | 'utf-8' | 'ucs2' | 'ucs-2' | 'utf16le' | 'utf-16le'

3. I haven't tested it myself, but it seems that cy.fixture would work correctly if null is given as encoding. I think this should be added to the cy.fixture doc, too. As we all know, this issue started with cy.fixture problem.

packages/driver/cypress/integration/commands/files_spec.js

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

BlueWinds · 2021-10-19T16:54:39Z

User facing changelog

When { encoding: null } is passed to cy.readFile(), the file is treated as binary and read as a Buffer. Similarly, { encoding: null } passed to cy.writeFile() allows direct writing of buffers.

Although { encoding: value } works, it is recommended to pass encoding as the second arg. So, it might be better to change this like:

When null is passed to encoding of cy.readFile(), the file is treated as binary and read as a Buffer. Similarly, null passed to encoding of cy.writeFile() allows direct writing of buffers.

Done.

Add null to Encodings types.

cypress/cli/types/cypress.d.ts

Line 5675 in a045e4f

type Encodings = 'ascii' | 'base64' | 'binary' | 'hex' | 'latin1' | 'utf8' | 'utf-8' | 'ucs2' | 'ucs-2' | 'utf16le' | 'utf-16le'

Done.

I haven't tested it myself, but it seems that cy.fixture would work correctly if null is given as encoding. I think this should be added to the cy.fixture doc, too. As we all know, this issue started with cy.fixture problem.

It doesn't work with fixtures at the moment. I'll fix it in an upcoming commit here - the behavior should definitely be symetrical between read/writeFile and fixture.

This reverts commit c7e5ff6.

sainthkh

LGTM.

I just left a comment about the missing comment.

I don't have authority to commit this PR. So, someone in the team will do the job.

packages/driver/cypress/integration/commands/fixtures_spec.js

sainthkh

These comments should be changed or they'll be unclear.

packages/server/test/unit/fixture_spec.js

sainthkh

LGTM. 👍

packages/driver/cypress/integration/commands/assertions_spec.js

packages/driver/cypress/integration/commands/navigation_spec.js

packages/server/lib/files.js

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

…coding-for-binary-files

BlueWinds · 2021-10-25T19:53:59Z

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

+        if (options.encoding === null) {
+          response = Buffer.from(response)
+        } else if (response instanceof ArrayBuffer) {
+          // Cypress' behavior is to base64 encode binary files if the user


@flotwig - As part of removing the server-side transform of buffers to base64 before transmitting (based on your review, which I 100% agree with), we now have to explicitly do so on the driver side unless a null encoding is specified, in order to maintain backwards compatibility.

In a vacuum it would make more conceptual sense to remove the default of utf8 for undefined encoding (and unknown file types) and base64 encoding for binary files, but that would turn this into a breaking change, so instead I ended up with this fairly messy set of conditionals spread around the driver in order to maintain backwards compatibility.

In a vacuum it would make more conceptual sense to remove the default of utf8 for undefined encoding (and unknown file types) and base64 encoding for binary files, but that would turn this into a breaking change

Yeah, and I wonder if that breaking change would even be welcome. I would guess that many people are relying on the existing behavior that treats everything as utf8 by default.

flotwig · 2021-10-26T14:54:56Z

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

+        if (options.encoding === null) {
+          response = Buffer.from(response)
+        } else if (response instanceof ArrayBuffer) {
+          // Cypress' behavior is to base64 encode binary files if the user


In a vacuum it would make more conceptual sense to remove the default of utf8 for undefined encoding (and unknown file types) and base64 encoding for binary files, but that would turn this into a breaking change

Yeah, and I wonder if that breaking change would even be welcome. I would guess that many people are relying on the existing behavior that treats everything as utf8 by default.

cypress-bot · 2021-11-10T21:50:23Z

Released in 9.0.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v9.0.0, please open a new issue.

Allow explicit null encoding for readFile/writeFile

2e52a2b

BlueWinds changed the title ~~Allow explicit null encoding for readFile/writeFile~~ feat: Allow explicit null encoding for readFile/writeFile Oct 18, 2021

BlueWinds marked this pull request as ready for review October 18, 2021 19:49

BlueWinds requested a review from a team as a code owner October 18, 2021 19:49

BlueWinds requested review from mjhenkes, emilyrohrbough, sainthkh and flotwig and removed request for a team, mjhenkes and emilyrohrbough October 18, 2021 19:49

sainthkh suggested changes Oct 19, 2021

View reviewed changes

packages/driver/cypress/integration/commands/files_spec.js Show resolved Hide resolved

packages/driver/src/cy/commands/files.ts Show resolved Hide resolved

BlueWinds added 5 commits October 19, 2021 11:59

Add 'null' encoding for fixtures, comment code more

7987558

Fix cy.intercept() to previous behavior

1936a71

More attempts to get fixtures working properly

6943186

Once more with feeling

5a9c52c

Adjust server unit tests to new behavior

450df2f

BlueWinds requested a review from sainthkh October 20, 2021 17:30

BlueWinds added 3 commits October 20, 2021 10:37

Add retries to potentially flaky driver specs

c7e5ff6

Revert "Add retries to potentially flaky driver specs"

ff0b932

This reverts commit c7e5ff6.

Add retries to flaky specs

9005a38

sainthkh approved these changes Oct 21, 2021

View reviewed changes

packages/driver/cypress/integration/commands/fixtures_spec.js Show resolved Hide resolved

sainthkh suggested changes Oct 21, 2021

View reviewed changes

packages/server/test/unit/fixture_spec.js Outdated Show resolved Hide resolved

sainthkh reviewed Oct 21, 2021

View reviewed changes

packages/server/test/unit/fixture_spec.js Show resolved Hide resolved

BlueWinds added 2 commits October 21, 2021 09:16

Update test titles

065609a

Final missed comment

2bacf10

BlueWinds mentioned this pull request Oct 22, 2021

Add explicit null as an option for encoding cypress-io/cypress-documentation#4172

Merged

sainthkh previously approved these changes Oct 25, 2021

View reviewed changes

flotwig suggested changes Oct 25, 2021

View reviewed changes

Simplification based on review comments

14ab9ec

BlueWinds dismissed sainthkh’s stale review via 14ab9ec October 25, 2021 18:31

BlueWinds added 4 commits October 25, 2021 11:34

Remove retries in favor of waiting on correct global solution

505d8f3

Fix missed test in last pass

e6767da

Merge remote-tracking branch 'origin/develop' into issue-1558-null-en…

96aaac3

…coding-for-binary-files

Restore previous behavior to binary fixtures with no encoding specified

5905fdf

BlueWinds commented Oct 25, 2021

View reviewed changes

BlueWinds requested a review from flotwig October 25, 2021 20:40

flotwig approved these changes Oct 26, 2021

View reviewed changes

BlueWinds merged commit 3b3b7f8 into develop Oct 26, 2021

BlueWinds deleted the issue-1558-null-encoding-for-binary-files branch October 26, 2021 15:42

cypress-bot bot mentioned this pull request Oct 26, 2021

reading binary file from fixture differs from drop event file #1558

Closed

BlueWinds mentioned this pull request Oct 26, 2021

[Feature] Cypress adding native attach file support in 9.3.0 abramenal/cypress-file-upload#338

Closed

cypress-bot bot locked as resolved and limited conversation to collaborators Nov 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow explicit null encoding for readFile/writeFile #18534

feat: Allow explicit null encoding for readFile/writeFile #18534

BlueWinds commented Oct 18, 2021 •

edited

Loading

cypress-bot bot commented Oct 18, 2021

cypress bot commented Oct 18, 2021 •

edited

Loading

BlueWinds commented Oct 18, 2021

sainthkh left a comment •

edited

Loading

BlueWinds commented Oct 19, 2021

sainthkh left a comment •

edited

Loading

sainthkh left a comment

sainthkh left a comment •

edited

Loading

BlueWinds Oct 25, 2021 •

edited

Loading

flotwig Oct 26, 2021

flotwig Oct 26, 2021

cypress-bot bot commented Nov 10, 2021

feat: Allow explicit null encoding for readFile/writeFile #18534

feat: Allow explicit null encoding for readFile/writeFile #18534

Conversation

BlueWinds commented Oct 18, 2021 • edited Loading

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Oct 18, 2021

cypress bot commented Oct 18, 2021 • edited Loading

Test summary

Run details

BlueWinds commented Oct 18, 2021

sainthkh left a comment • edited Loading

Choose a reason for hiding this comment

BlueWinds commented Oct 19, 2021

sainthkh left a comment • edited Loading

Choose a reason for hiding this comment

sainthkh left a comment

Choose a reason for hiding this comment

sainthkh left a comment • edited Loading

Choose a reason for hiding this comment

BlueWinds Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

flotwig Oct 26, 2021

Choose a reason for hiding this comment

flotwig Oct 26, 2021

Choose a reason for hiding this comment

cypress-bot bot commented Nov 10, 2021

BlueWinds commented Oct 18, 2021 •

edited

Loading

cypress bot commented Oct 18, 2021 •

edited

Loading

sainthkh left a comment •

edited

Loading

sainthkh left a comment •

edited

Loading

sainthkh left a comment •

edited

Loading

BlueWinds Oct 25, 2021 •

edited

Loading