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

execa (cy.exec) doesn't respect $PATH variable #104

Closed
Robdel12 opened this issue May 14, 2019 · 4 comments · Fixed by #140
Closed

execa (cy.exec) doesn't respect $PATH variable #104

Robdel12 opened this issue May 14, 2019 · 4 comments · Fixed by #140
Assignees

Comments

@Robdel12
Copy link
Contributor

What is this?

There's an issue I've been seeing with cy.exec. They use execa under the hood and it doesn't respect your systems $PATH variable. Various customers get an error that basically says it couldn't find node on their system. This is because execa looking in one specific place for the node executable. Related issue: sindresorhus/execa#153

We use cy.exec to run the health check so we can ensure our node server is open and ready to accept the snapshot POSTs: https://github.com/percy/percy-cypress/blob/master/lib/index.ts#L12-L17

Work around

The work around here would be to symlink your systems node to where execa is expecting to find it:

$  sudo ln -s $(which node) /usr/bin/node

Long term proper fix

Not quite sure about this. Contribute back to execa to fix it? Figure out a better way to run the health check?

@nachin
Copy link

nachin commented Jul 3, 2019

my system node is located in /usr/local/bin/node. I am a bit confused as to where I should set the symlink path to for execa.

Thanks in advance

@Robdel12
Copy link
Contributor Author

Robdel12 commented Jul 3, 2019

Hey @nachin did you try running sudo ln -s $(which node) /usr/bin/node? That should symlink the path your node is really installed at to the path execa is expecting

@nachin
Copy link

nachin commented Jul 3, 2019

yeah I tried running that verbatim and got "Operation not permitted" running OS X Mojave.

sudo ln -s $(which node) /usr/local/bin/node , yields File exists

@Robdel12
Copy link
Contributor Author

Hey @nachin, would it be possible you're hitting this issue? https://superuser.com/questions/933019/sudo-cant-create-file-in-usr-bin-in-el-capitan

Robdel12 added a commit that referenced this issue Aug 2, 2019
This will fix #104 (and hopefully #138).

`cy.task` will always execute within the version of node that is bundled with
Cypress, so we no longer have to worry about `$PATH` issues that `cy.exec`
faces. We're also no longer running a CLI command for the health check, this new
implementation will make a HTTP request from node.

This does mean we need to update the docs to let users know there's an extra
step to configure the SDK properly now.

In an ideal world we would be able to `.catch` on `cy.request` and disable Percy
after the first failed `POST` of the DOM. I think in the future we should look
into working with Cypress to implement this so we can shrink the integration,
make the SDK more reliable, and faster (since we won't make 2 network requests
per-snapshot).
Robdel12 added a commit that referenced this issue Aug 2, 2019
…HANGE) (#140)

BREAKING CHANGE:

## The problem

In all of the Percy SDKs we do a thing called a "heath check", which makes sure the Percy agent server is open and ready to accept the DOM we're going to `POST` to it. If the health check fails, we will disable Percy in the SDK so we're not failing your tests due to Percy not running. 

In the Cypress SDK, this was implemented in an interesting way due a limitation with `cy.request`.  The TL:DR of that is we can't `.catch` a failed request, so we needed to use `cy.exec` to health check & gracefully fall out. You can read a little more about that limitation here: cypress-io/cypress#3161

`cy.exec` has it's own issues, which are outlined here: #104 This is a major blocker for _a lot_ of customers. 

## What is this?

This will fix #104 (and hopefully #138).

`cy.task` will always execute within the version of node that is bundled with Cypress, so we no longer have to worry about `$PATH` issues that `cy.exec` faces. We're also no longer running a CLI command for the health check, this new implementation will make a HTTP request from node.

This does mean we need to update the docs to let users know there's an extra step to configure the SDK properly now.

### Upgrading to v2.x

With this change, you will now need to import this new task into your projects plugins (`cypress/plugins/index.js`) file. Without that, the SDK will not work at all. 


```js
/// In cypress/plugins/index.js

let percyHealthCheck = require('@percy/cypress/task')

module.exports = (on, config) => {
  // `on` is used to hook into various events Cypress emits
  // `config` is the resolved Cypress config

  // Make it possible to log things to stdout by calling 'cy.task('log', 'some message to log').
  // Useful for development and debugging.
  on("task", {
    log(message) {
      console.log(message);
      return null;
    }
  });
  on("task", percyHealthCheck);
};
```

## In the future

In an ideal world we would be able to `.catch` on `cy.request` and disable Percy after the first failed `POST` of the DOM. I think in the future we should look into working with Cypress to implement this so we can shrink the integration, make the SDK more reliable, and faster (since we won't make 2 network requests per-snapshot).

## FAQ

#### Why can't you use `fetch` over `cy.request` & `.catch` that?

We need to use `cy.request` since those requests aren't actually executed in the browser and avoid any CORS issues. 

## TODOs

- [x] Update Docs
- [x] How can I guarantee semantic release makes this a major version bump 
- [x] How does one integrate this task into their suite? It's not a default export.. And it runs in node. Maybe it can be apart of the default export
djones pushed a commit that referenced this issue Aug 2, 2019
# [2.0.0](v1.0.9...v2.0.0) (2019-08-02)

### Bug Fixes

* Use `cy.task` for health check rather than `cy.exec` (BREAKING CHANGE) ([#140](#140)) ([40550f7](40550f7)), closes [#104](#104) [#104](#104) [#138](#138)

### BREAKING CHANGES

* ## The problem

In all of the Percy SDKs we do a thing called a "heath check", which makes sure the Percy agent server is open and ready to accept the DOM we're going to `POST` to it. If the health check fails, we will disable Percy in the SDK so we're not failing your tests due to Percy not running.

In the Cypress SDK, this was implemented in an interesting way due a limitation with `cy.request`.  The TL:DR of that is we can't `.catch` a failed request, so we needed to use `cy.exec` to health check & gracefully fall out. You can read a little more about that limitation here: cypress-io/cypress#3161
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 a pull request may close this issue.

2 participants