Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Informative error when invalid CYPRESS_INSTALL_BINARY #5060

Closed
wants to merge 1 commit into from
Closed

Informative error when invalid CYPRESS_INSTALL_BINARY #5060

wants to merge 1 commit into from

Conversation

tomasbjerre
Copy link
Contributor

@tomasbjerre tomasbjerre commented Aug 29, 2019

I had CYPRESS_INSTALL_BINARY set to 'http://serrver.com/' (including single quotes). And I did not get an informative error stating that this was the case.

Pre-merge Tasks

  • Have tests been added/updated for the changes in this PR?

@tomasbjerre tomasbjerre changed the title Better information when invalid use of CYPRESS_INSTALL_BINARY Informative error when invalid CYPRESS_INSTALL_BINARY Aug 29, 2019
I had CYPRESS_INSTALL_BINARY set to "'http://serrver.com/'" (including
single quotes). And I did not get an informative error stating that this
was the case.

Closes #5059
Closes #5062
@@ -128,6 +128,31 @@ const downloadAndUnzip = ({ version, installDir, downloadDir }) => {
return Promise.resolve(tasks.run())
}

const INSTALL_BINARY_TYPES = {
URL: (c) => /^https?:\/\/.*/.test(c),
FS: (c) => fs.pathExistsSync(c),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

due to file system on Mac locking up with too many M files, we use gracefulfs to retry file access. Which means that ALL file operations have to be async, and we should never use ...Sync versions. Could you change this code to check for file existence to be async, please?

return throwFormErrorText(errors.cypressInstallBinaryInvalid)(needVersion)
}

debug(`Identified CYPRESS_INSTALL_BINARY as type: ${needVersionType}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this work? What does it print - the function name?

@tomasbjerre
Copy link
Contributor Author

Open a new PR to just solve #5062 : #5125

Closing this one as I think this code needs a major refactoring. Opened this issue to try to discus how: #5126

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Informative error when invalid CYPRESS_INSTALL_BINARY
2 participants