cli: unnzip Cypress using `unzip` utility on Linux #5851

flotwig · 2019-12-02T22:58:36Z

Fixes "Unzipping Cypress" takes four minutes #3803 on Linux

User facing changelog

Cypress will attempt to use the unzip binary during cypress install before trying the slower, Node.js-based unzipping method.

Additional details

time spent unzipping (unzipMs):
- with node.js unzipping: 25667ms
- with unzip: 5320ms
- will definitely help people with slow CI
technically, unzip could work on macOS or windows, but it's probably not going to exist on either so I'm not sure we should even attempt it

How has the user experience changed?

cypress install is much faster

PR Tasks

Have tests been added/updated?
- not sure how to test - existing ditto stuff doesn't have a test
- I have written a test for a situation when unzip is unavailable, but had to skip it - it was causing weird verify_spec failures
Has the original issue been tagged with a release in ZenHub?

cypress-bot · 2019-12-02T22:58:38Z

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.

cypress · 2019-12-02T23:11:19Z

Test summary

3517 • 0 • 45 • 0

Run details


Project	cypress
Status	Passed
Commit	`7328ea3`
Started	Dec 3, 2019 12:38 AM
Ended	Dec 3, 2019 12:42 AM
Duration	03:51 💡
OS	Linux Debian - 9.8
Browser	Multiple

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

bahmutov · 2019-12-02T23:17:33Z

cli/lib/tasks/unzip.js

@@ -113,13 +157,20 @@ const unzip = ({ zipFilePath, installDir, progress }) => {
          case 'darwin':
            return unzipWithOsx()
          case 'linux':
+            return unzipWithUnzipTool()


but will every Linux system include unzip?

no, but it silently falls back to node.js unzip if it fails

bahmutov · 2019-12-02T23:21:06Z

cli/lib/tasks/unzip.js

+
+          const sp = cp.spawn('unzip', ['-o', zipFilePath, '-d', installDir])
+
+          sp.on('error', unzipWithNode)


oh, I see you fall to Node in that case

ha // f-it just unzip with node

i believe that is your comment 😆

bahmutov · 2019-12-02T23:42:44Z

shell we start adding unzip to our docker images?

flotwig · 2019-12-02T23:45:45Z

shell we start adding unzip to our docker images?

cypress/browsers:node12.0.0-chrome73 seems to have it already, so it might already be in others

bahmutov · 2019-12-03T00:49:49Z

I added a sample test, but what really bothers me is that it works by itself, yet when all the tests run together - there are 4 tests in verify that fail with weird errors, seems like some promise somewhere is left hanging

bahmutov · 2019-12-03T14:14:03Z

This is pretty sweet - this PR unzips on Circle in 21 seconds, while develop branch runs same job in 38 seconds.

bahmutov

I like this PR and thinks it achieves its purpose. I wish we did another pass at refactoring and moved each "unzip" function into own functions we can spy on / stub from tests. Right now they all live in shared single large function, but that's ok for now, keeps change to a minimum

* unzip using 'unzip' utility on linux * add example unzip test

unzip using 'unzip' utility on linux

0a6216d

flotwig requested review from bahmutov and kuceb December 2, 2019 23:10

bahmutov reviewed Dec 2, 2019

View reviewed changes

flotwig mentioned this pull request Dec 2, 2019

"Unzipping Cypress" takes four minutes #3803

Closed

add example unzip test

7328ea3

bahmutov approved these changes Dec 3, 2019

View reviewed changes

bahmutov changed the title ~~Unzip Cypress using unzip utility on Linux~~ cli: unnzip Cypress using unzip utility on Linux Dec 3, 2019

bahmutov merged commit 02515fe into develop Dec 3, 2019

flotwig mentioned this pull request Dec 3, 2019

Improve unzip time on Windows #5861

Closed

santoshyadavdev pushed a commit to santoshyadavdev/cypress that referenced this pull request Dec 5, 2019

cli: unnzip Cypress using unzip utility on Linux (cypress-io#5851)

45fa76e

* unzip using 'unzip' utility on linux * add example unzip test

avallete pushed a commit to avallete/cypress that referenced this pull request Dec 10, 2019

cli: unnzip Cypress using unzip utility on Linux (cypress-io#5851)

9bda308

* unzip using 'unzip' utility on linux * add example unzip test

goodliff mentioned this pull request Dec 16, 2019

Unzipping progress goes to 200% #5967

Closed

flotwig deleted the issue-3803-linux-native-unzip branch January 24, 2022 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: unnzip Cypress using `unzip` utility on Linux #5851

cli: unnzip Cypress using `unzip` utility on Linux #5851

flotwig commented Dec 2, 2019 •

edited by bahmutov

cypress-bot bot commented Dec 2, 2019

cypress bot commented Dec 2, 2019 •

edited

bahmutov Dec 2, 2019

flotwig Dec 2, 2019

bahmutov Dec 2, 2019

bahmutov Dec 2, 2019

flotwig Dec 2, 2019

bahmutov commented Dec 2, 2019

flotwig commented Dec 2, 2019

bahmutov commented Dec 3, 2019

bahmutov commented Dec 3, 2019

bahmutov left a comment


		const sp = cp.spawn('unzip', ['-o', zipFilePath, '-d', installDir])

		sp.on('error', unzipWithNode)

cli: unnzip Cypress using unzip utility on Linux #5851

cli: unnzip Cypress using unzip utility on Linux #5851

Conversation

flotwig commented Dec 2, 2019 • edited by bahmutov

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Dec 2, 2019

PR Review Checklist

User Experience

Functionality

Maintainability

Quality

Internal

cypress bot commented Dec 2, 2019 • edited

Test summary

Run details

bahmutov Dec 2, 2019

Choose a reason for hiding this comment

flotwig Dec 2, 2019

Choose a reason for hiding this comment

bahmutov Dec 2, 2019

Choose a reason for hiding this comment

bahmutov Dec 2, 2019

Choose a reason for hiding this comment

flotwig Dec 2, 2019

Choose a reason for hiding this comment

bahmutov commented Dec 2, 2019

flotwig commented Dec 2, 2019

bahmutov commented Dec 3, 2019

bahmutov commented Dec 3, 2019

bahmutov left a comment

Choose a reason for hiding this comment

cli: unnzip Cypress using `unzip` utility on Linux #5851

cli: unnzip Cypress using `unzip` utility on Linux #5851

flotwig commented Dec 2, 2019 •

edited by bahmutov

cypress bot commented Dec 2, 2019 •

edited