Issue 1281 warning on no write #7126

mrmeku · 2020-04-23T20:26:30Z

Closes Cypress should not require write access of root folder #1281

User facing changelog

Cypress no longer requires write access to the projectRoot, it instead will display a warning.

Additional details

This allows for cypress inetegration with other tools such as Bazel
It allows Cypress to be run in sandboxed or otherwise readonly fs environments

How has the user experience changed?

Before

After

PR Tasks

Have tests been added/updated?
Has the original issue been tagged with a release in ZenHub?
Has a PR for user-facing changes been opened in cypress-documentation?

cypress-bot · 2020-04-23T20:26:32Z

Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.

Please write [WIP] in the title of your Pull Request if your PR is not ready for review - someone will review your PR as soon as the [WIP] is removed.
Please familiarize yourself with the PR Review Checklist and feel free to make updates on your PR based on these guidelines.

PR Review Checklist

If any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'.

User Experience

The feature/bugfix is self-documenting from within the product.
The change provides the end user with a way to fix their problem (no dead ends).

Functionality

The code works and performs its intended function with the correct logic.
Performance has been factored in (for example, the code cleans up after itself to not cause memory leaks).
The code guards against edge cases and invalid input and has tests to cover it.

Maintainability

The code is readable (too many nested 'if's are a bad sign).
Names used for variables, methods, etc, clearly describe their function.
The code is easy to understood and there are relevant comments explaining.
New algorithms are documented in the code with link(s) to external docs (flowcharts, w3c, chrome, firefox).
There are comments containing link(s) to the addressed issue (in tests and code).

Quality

The change does not reimplement code.
There's not a module from the ecosystem that should be used instead.
There is no redundant or duplicate code.
There are no irrelevant comments left in the code.
Tests are testing the code’s intended functionality in the best way possible.

Internal

The original issue has been tagged with a release in ZenHub.

CLAassistant · 2020-04-23T20:26:38Z

All committers have signed the CLA.

mrmeku · 2020-04-28T17:45:42Z

@jennifer-shehane Could I get a review on this? Its a continuation of a PR you were reviewing: #5584

packages/server/test/e2e/8_read_only_fs.ts

packages/server/lib/errors.coffee

mrmeku · 2020-05-04T18:22:24Z

@jennifer-shehane Merged your changes! Thanks for those. Is there anything else you'd like me to do before this is ready to be merged?

jennifer-shehane

When you add snapshots, this generates a file of the stdout output of the test you wrote. So, you need to run SNAPSHOT_UPDATE=1 yarn test-e2e 8_read_only to generate this file for comparison later (I've gone ahead and done this and committed it). The idea is - if something breaks, we'll see the changes in the STDOUT.

Since the test you wrote has randomized characters - this won't work for snapshot testing. Each time you run the test, the generated output will be different. We need the test to have the same output each time the test is run.

packages/server/test/e2e/8_read_only_fs.ts

mrmeku · 2020-05-06T22:36:36Z

@jennifer-shehane Snapshots updated and ready to go

jennifer-shehane

There's an issue with the yarn lockfile that was committed. It's resolving to the incorrect has-binary dep. The has-binary dep should not have updated in your committed lockfile.

mrmeku · 2020-05-07T23:24:45Z

@jennifer-shehane Sorry I didn't catch that when I was reviewing my code. It's resolved now

flotwig · 2020-05-08T13:30:01Z

packages/server/lib/util/settings.js

@@ -126,7 +126,11 @@ module.exports = {
        throw err
      }

-      // else we cannot read due to folder permissions
+      if (err.toString().startsWith('Error: EACCES: permission denied')) {


See how line 119 uses an object to filter the .catch block. You should be able to do this in a similar fashion:

.catch({ code: 'EACCES' }, () => { return errors.warning(...) })

done! thanks for the advise

flotwig · 2020-05-08T13:30:46Z

packages/server/package.json

-}
+}


revert this plz. you may want to turn on the preference to "end files with a newline" in your editor

packages/server/test/e2e/8_read_only_fs.ts

flotwig · 2020-05-08T13:48:09Z

packages/server/test/e2e/8_read_only_fs.ts

+    // the dir needs executable for debugging
+    // the main test here is that it isn't writable, which would be either 6 or 7
+    // See here for more info on these numbers http://permissions-calculator.org/
+    chmodr.sync(projectDir, 0o500)


whoa, i didn't know this 0o prefix was a thing, i am only familiar with using octals via the leading 0 (like 0500)

Yeah, you totally have to do it this way when running in strict mode
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Deprecated_octal

Learn something new about java script everyday

packages/server/test/e2e/8_read_only_fs.ts

flotwig · 2020-05-08T13:49:42Z

packages/server/test/e2e/8_read_only_fs.ts

+const e2e = require('../support/helpers/e2e')
+const chmodr = require('chmodr')
+
+describe('e2e project dir access', function () {


this is a nice, robust test. you should rename the file to 8_read_only_fs_spec.ts to match how other e2e tests are named.

flotwig · 2020-05-08T13:54:19Z

packages/server/__snapshots__/8_read_only_fs.ts.js

+Warning: We failed to record the video.
+
+This error will not alter the exit code.
+
+Error: ffmpeg exited with code 1: /tmp/8_read_only_fs/cypress/videos/spec.js.mp4: Permission denied
+
+      [stack trace lines]
+
+Warning: We failed processing this video.
+
+This error will not alter the exit code.
+
+Error: ffmpeg exited with code 1: /tmp/8_read_only_fs/cypress/videos/spec.js.mp4: Permission denied
+
+      [stack trace lines]


would also like to see a screenshot failure here... using a spec that fails should generate a screenshot

This change replaces the error with a warning when cypress is executing tests in a readonly fs.

- don't need to run test in a specific browser - can combine SS + video test into one - don't seem to need readonly_fs_spec.js

flotwig · 2020-05-11T14:37:55Z

Nyah... the e2e tests are failing because CI runs as root, and root doesn't get a permission denied error. Let me see how we can work around this.

flotwig

Nice, I did some cleanup and made it so that failure to create the videos directory will also not crash the test. Added a new e2e test stage so we can run the tests as non-root as well.

circle.yml

packages/server/lib/errors.coffee

flotwig · 2020-05-11T18:55:32Z

Note that this PR will not fix errors caused by failure to "scaffold" a new project with a "supportFile". Some work needs to be done to not scaffold supportFolder in run mode:

https://github.com/mrmeku/cypress/blob/b5e46dd3bc7de54b4f9234754e0228ab64a9294e/packages/server/lib/project.js#L574-L582

And with that, it will need to be made possible for individual scaffolding steps to fail and cause the default values for config.supportFile/etc. to change, otherwise Cypress will just end up looking for non-existent files if scaffolding fails.

packages/server/lib/util/settings.js

packages/server/test/support/fixtures/projects/read-only-project-root/cypress/plugins/index.js

bahmutov

I think this is good fix

Dismissing my previous review

jennifer-shehane · 2020-05-12T07:54:46Z

@flotwig @bahmutov Can this PR be approved?

jennifer-shehane · 2020-05-12T08:05:06Z

@flotwig Are you saying that this PR will not fully close #1281?

flotwig · 2020-05-12T13:12:39Z

@jennifer-shehane it's good to go, just waiting for those dang contributor tests to pass 🙄

It closes #1281 since it will allow the folks in that thread to use Cypress on a read-only FS. I'll open a more specific issue about scaffolding requiring write access once this is closed. (opened #7310)

yezhengli-Mr9 · 2023-11-30T18:53:48Z

Still #1281

See discussion and possible solutions at
npm ERR! https://github.com/cypress-io/cypress/issues/1281
npm ERR! 
npm ERR! ----------
npm ERR! 
npm ERR! Failed to access /root/.cache/Cypress:
npm ERR! 
npm ERR! EACCES: permission denied, mkdir '/root/.cache/Cypress'

although tried

 sudo chown -R $USER:$GROUP ~/.npm
sudo chmod +x /root/.cache

jennifer-shehane · 2023-12-01T18:15:38Z

@yezhengli-Mr9 Please open a new issue if you're encountering an issue with Cypress filling out the template.

mrmeku changed the title ~~Carry on my toxicble son~~ Issue 1281 warning on no write Apr 23, 2020

mrmeku force-pushed the carry-on-my-toxicble-son branch 4 times, most recently from ffca899 to 201ce45 Compare April 24, 2020 19:03

jennifer-shehane requested changes Apr 29, 2020

View reviewed changes

packages/server/test/e2e/8_read_only_fs.ts Outdated Show resolved Hide resolved

packages/server/lib/errors.coffee Outdated Show resolved Hide resolved

mrmeku force-pushed the carry-on-my-toxicble-son branch 2 times, most recently from d2aa498 to 34e5e6d Compare May 4, 2020 18:21

mrmeku requested a review from jennifer-shehane May 4, 2020 18:22

jennifer-shehane requested changes May 5, 2020

View reviewed changes

packages/server/test/e2e/8_read_only_fs.ts Outdated Show resolved Hide resolved

mrmeku force-pushed the carry-on-my-toxicble-son branch from c99e6ad to 78a3f52 Compare May 6, 2020 22:35

mrmeku requested a review from jennifer-shehane May 6, 2020 22:36

jennifer-shehane previously requested changes May 7, 2020

View reviewed changes

mrmeku force-pushed the carry-on-my-toxicble-son branch from 78a3f52 to 3dae576 Compare May 7, 2020 23:24

mrmeku requested a review from jennifer-shehane May 7, 2020 23:25

jennifer-shehane requested review from bahmutov and flotwig May 8, 2020 10:36

flotwig suggested changes May 8, 2020

View reviewed changes

replace fs write error with warning

8f6d58d

This change replaces the error with a warning when cypress is executing tests in a readonly fs.

mrmeku force-pushed the carry-on-my-toxicble-son branch from 08cd6a2 to 8f6d58d Compare May 9, 2020 19:20

jennifer-shehane requested a review from flotwig May 11, 2020 07:48

flotwig added 2 commits May 11, 2020 10:05

some cleanup

8df993b

- don't need to run test in a specific browser - can combine SS + video test into one - don't seem to need readonly_fs_spec.js

if videos folder cannot be created, just warn

d2287f9

flotwig force-pushed the carry-on-my-toxicble-son branch from 85a8019 to cd368ca Compare May 11, 2020 16:11

try a non-root e2e test stage

b5e46dd

flotwig force-pushed the carry-on-my-toxicble-son branch from cd368ca to b5e46dd Compare May 11, 2020 16:46

flotwig previously approved these changes May 11, 2020

View reviewed changes

bahmutov reviewed May 11, 2020

View reviewed changes

circle.yml Show resolved Hide resolved

bahmutov reviewed May 11, 2020

View reviewed changes

packages/server/lib/errors.coffee Outdated Show resolved Hide resolved

bahmutov reviewed May 11, 2020

View reviewed changes

packages/server/lib/util/settings.js Outdated Show resolved Hide resolved

bahmutov reviewed May 11, 2020

View reviewed changes

packages/server/test/support/fixtures/projects/read-only-project-root/cypress/plugins/index.js Show resolved Hide resolved

bahmutov previously approved these changes May 11, 2020

View reviewed changes

fix comment

3e9a2b8

flotwig dismissed stale reviews from bahmutov and themself via 3e9a2b8 May 11, 2020 19:08

flotwig force-pushed the carry-on-my-toxicble-son branch from a76e59a to 62b4bc9 Compare May 11, 2020 19:24

FOLDER_NOT_ACCESSIBLE

2de743b

flotwig force-pushed the carry-on-my-toxicble-son branch from 62b4bc9 to 2de743b Compare May 11, 2020 19:37

jennifer-shehane mentioned this pull request May 12, 2020

Unable to open cypress UI #7243

Closed

flotwig approved these changes May 12, 2020

View reviewed changes

flotwig merged commit f5284c6 into cypress-io:develop May 12, 2020

cypress-bot bot mentioned this pull request May 12, 2020

Cypress should not require write access of root folder #1281

Closed

flotwig mentioned this pull request May 12, 2020

Remove need for write permissions during scaffolding #7310

Closed

flotwig mentioned this pull request Oct 13, 2021

Fix warning when projectRoot is not writable #18485

Closed

varac mentioned this pull request Jun 24, 2022

Cypress in docker fails to run with read-only project dir cypress-io/cypress-docker-images#686

Open

cypress-io locked and limited conversation to collaborators Dec 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 1281 warning on no write #7126

Issue 1281 warning on no write #7126

mrmeku commented Apr 23, 2020 •

edited by jennifer-shehane

Loading

cypress-bot bot commented Apr 23, 2020

CLAassistant commented Apr 23, 2020 •

edited

Loading

mrmeku commented Apr 28, 2020

mrmeku commented May 4, 2020

jennifer-shehane left a comment

mrmeku commented May 6, 2020

jennifer-shehane left a comment

mrmeku commented May 7, 2020

flotwig May 8, 2020

mrmeku May 9, 2020

flotwig May 8, 2020

flotwig May 8, 2020

mrmeku May 9, 2020

flotwig May 8, 2020

flotwig May 8, 2020

flotwig commented May 11, 2020

flotwig left a comment

flotwig commented May 11, 2020

bahmutov left a comment

jennifer-shehane commented May 12, 2020

jennifer-shehane commented May 12, 2020

flotwig commented May 12, 2020 •

edited

Loading

yezhengli-Mr9 commented Nov 30, 2023 •

edited

Loading

jennifer-shehane commented Dec 1, 2023

Issue 1281 warning on no write #7126

Issue 1281 warning on no write #7126

Conversation

mrmeku commented Apr 23, 2020 • edited by jennifer-shehane Loading

User facing changelog

Additional details

How has the user experience changed?

Before

After

PR Tasks

cypress-bot bot commented Apr 23, 2020

PR Review Checklist

User Experience

Functionality

Maintainability

Quality

Internal

CLAassistant commented Apr 23, 2020 • edited Loading

mrmeku commented Apr 28, 2020

mrmeku commented May 4, 2020

jennifer-shehane left a comment

Choose a reason for hiding this comment

mrmeku commented May 6, 2020

jennifer-shehane left a comment

Choose a reason for hiding this comment

mrmeku commented May 7, 2020

flotwig May 8, 2020

Choose a reason for hiding this comment

mrmeku May 9, 2020

Choose a reason for hiding this comment

flotwig May 8, 2020

Choose a reason for hiding this comment

flotwig May 8, 2020

Choose a reason for hiding this comment

mrmeku May 9, 2020

Choose a reason for hiding this comment

flotwig May 8, 2020

Choose a reason for hiding this comment

flotwig May 8, 2020

Choose a reason for hiding this comment

flotwig commented May 11, 2020

flotwig left a comment

Choose a reason for hiding this comment

flotwig commented May 11, 2020

bahmutov left a comment

Choose a reason for hiding this comment

jennifer-shehane commented May 12, 2020

jennifer-shehane commented May 12, 2020

flotwig commented May 12, 2020 • edited Loading

yezhengli-Mr9 commented Nov 30, 2023 • edited Loading

jennifer-shehane commented Dec 1, 2023

mrmeku commented Apr 23, 2020 •

edited by jennifer-shehane

Loading

CLAassistant commented Apr 23, 2020 •

edited

Loading

flotwig commented May 12, 2020 •

edited

Loading

yezhengli-Mr9 commented Nov 30, 2023 •

edited

Loading