cypress-io / cypress

Fast, easy and reliable testing for anything that runs in a browser.
https://cypress.io
MIT License
46.87k stars 3.18k forks source link

Cypress and cypress-multi-reporters Yarn 2 PNP support #18922

Open massimeddu-sonic opened 2 years ago

massimeddu-sonic commented 2 years ago

Current behavior

After migrating my project to Yarn PNP, cypress is not able anymore to find cypress-multi-reporters, giving the following error message:

yarn cypress run --browser electron

Could not load reporter by name: cypress-multi-reporters

We searched for the reporter in these paths:

- /src/clients/web/cypress-multi-reporters
- /src/clients/web/node_modules/cypress-multi-reporters

The error we received was:

Cannot find module '/src/clients/web/node_modules/cypress-multi-reporters'
Require stack:
- /home/<user>/.cache/Cypress/9.0.0/Cypress/resources/app/packages/server/lib/reporter.js
- /home/<user>/.cache/Cypress/9.0.0/Cypress/resources/app/packages/server/lib/project-base.js
- /home/<user>/.cache/Cypress/9.0.0/Cypress/resources/app/packages/server/lib/open_project.js
- /home/<user>/.cache/Cypress/9.0.0/Cypress/resources/app/packages/server/lib/cypress.js
- /home/<user>/.cache/Cypress/9.0.0/Cypress/resources/app/packages/server/index.js
- /home/<user>/.cache/Cypress/9.0.0/Cypress/resources/app/index.js
- 

Learn more at https://on.cypress.io/reporters

Desired behavior

Cypress should be able to resolve the cypress-multi-reporters and run the tests correctly.

Test code to reproduce

yarn -v      
3.1.0
node --version
v16.13.0

package.json

"devDependencies": {
    "cypress": "^9.0.0",
    "cypress-multi-reporters": "^1.5.0",
  },

cypress.json

{
  "baseUrl": "http://localhost:3000",
  "videoCompression": false,
  "reporter": "cypress-multi-reporters",
  "reporterOptions": {
    "configFile": "cypress-reporter-config.json"
  }
}

Cypress Version

9.0.0

Other

No response

BlueWinds commented 2 years ago

@flotwig - mind taking a quick glance at this? I see you worked on #8008, so I have to imagine you know more here than my "looking up what is Yarn PNP".

It looks like while we support pnp as part of our standard webpack config, we may not support it for loading custom reporters - does that sound right?

flotwig commented 2 years ago

It looks like while we support pnp as part of our standard webpack config, we may not support it for loading custom reporters - does that sound right?

That's what it seems like, yeah. config.reporter is required here: https://github.com/cypress-io/cypress/blob/0b5f451a9bce73bb80db0f2d8e48d8f2ab7b772d/packages/server/lib/reporter.js#L530-L554

And this code path is not PNP aware. Needs to be made aware of yarn v2's PNP resolver when launched using yarn v2. Similar to #15623 's approach.

massimeddu-sonic commented 2 years ago

Hi @flotwig , thank you for having look at it. Following the approach of #15623 I've tried to change the line

p = path.resolve(projectRoot, reporterName)

with

p = require.resolve(reporterName, { paths: [projectRoot] })

but unfortunately it seams that it is not working. It looks like that yarn is not able to inject the PnP implementation of "require.resolve" when that code is executed. I don't know if it makes sense since I don't know the internals of cypress, but I suspect that is because this code is executed in the cypress server, and yarn calls the cypress CLI, so somehow the PNP implementation or require.resolve is lost when we pass from cypress CLI to cypress server.

Please let me know if you see any way or workaround to fix it, or if I can provide any other debug information.

Thank you,

massimeddu-sonic commented 2 years ago

Hi @flotwig

I finally realized that my fix actually works if the ELECTRON_RUN_AS_NODE=1 env variable is set. It still not work with the electron implementation of Cypress, but given that usually custom reporters are used in CI/CD pipelines in headless mode, it should cover most of the cases.

Fix is the PR: #19233

lexanth commented 2 years ago

The same issue affects me using Cypress in a monorepo with hoisting, but without cypress being run from the root. (without using PnP)

e.g. Cypress at /devtools/e2e with its own package.json

cypress-multi-reporters (or other NPM package reporters) get hoisted, so they're at /node_modules/cypress-multi-reporters. Cypress only looks for reporters at /devtools/e2e/node_modules/cypress-multi-reporters because it's looking for them manually rather than just using node's module resolution. In yarn 1, noHoist can be used to stop the reporter being hoisted. In yarn 2 (with node modules linker), there's no noHoist option.

My workaround is to create a custom reporter which just re-exports the reporter

module.exports = require('cypress-multi-reporters')

This require uses normal module resolution so correctly finds the hoisted version. Hacky workaround though.

kubijo commented 2 years ago

Is there any movement on this? I have just bumped into this issue because I'm trying to integrate cypress reports into a GitLab CI and none of the above solves it.

kubijo commented 2 years ago

PINGing with same question as above ... latest node & cypress, using typescript

ImSaSu commented 2 years ago

Similar to @lexanth we also run cypress-multi-reporters in a big monorepo using yarn2 workspaces (with the node modules linker).

@kubijo Our workaround is to create the symlink ourselves before executing cypress tests. We're only executing on linux, so we've added the following script to our package.json which is called by the test script to prepare for its execution:

{
    "scripts": {
        "link:cypress-dependency": "ln -sf ../../../node_modules/cypress-multi-reporters/",
        "test": "yarn link:cypress-dependency ; cypress run"
    }
}

Depending on your local folder structure, you may need to change the path (and probably have a test command with more parameters). I'm not sure if this helps with GitLab CI (as I've never used it), but it works on our jenkins agents without any issues.

We'd still love an official fix for this issue, as this workaround is undesirable.

kubijo commented 2 years ago

Thank you for the help, @ImSaSu, but we're standing firmly in the PnP linker land & of conviction that this should not be that much of a hassle to fix since, for example, Jest supports multiple reporters & works in our setup quite fine.

Arman92 commented 2 years ago

I also have this issue with yarn workspaces, cypress still looks for the reporter in package's node_module, however yarn has hoisted the reporter package to the root node_module.

public commented 1 year ago

The workaround I am using for this is to just use the relative path to the module. So instead of cypress-multi-reporters we have ../../node_modules/cypress-multi-reporters.

You could probably also work around this by doing reporter: require.resolve('cypress-multi-reporters') in your cypress.config.js.

Obviously this is hideous but 🤷

sfsepark commented 1 year ago

There have been attempts for pnp support in the 12.10.0 patch, but I still have issues on the reporter side. Looking at the Cypress internal code, I don't think there's a proper way to fix this outside of cypress.

https://github.com/cypress-io/cypress/pull/26452

lmiller1990 commented 1 year ago

This is the same issue as #25960. The fix should be the same, too - what needs to happen is before require is called, you need to have required the pnpapi module, which Yarn adds at runtime, and call setup(). That tampers with require and tells it how to resolve Yarn PnP packages, which are zipped (normally you cannot require a zipped module, that's what Yarn 3 needs to modify require).

Eg: https://github.com/cypress-io/cypress/pull/26452/files#diff-dce5144c067a639808029e38cef9e4e90a64dec50aeec74e6064f3cb31a56981R27-R33

The reason the fix in #25960 didn't apply here is the cypress.config is run in a different process to where the require(reporter) call runs. We need to do the same Yarn PnP require thing in the main Cypress process, too, not just the child process where cypress.config runs.

This is not possible in userland, I think we need to make a PR here. Luckily it should be easy, since you can just reuse the code I wrote in #26452.

If anyone would like to make a PR to this meaning, that'd be great. I can review it, along with some others from the Cypress team. What you'll need to do is add a line or two above here that does the same thing I do in this PR. You should be able to import the methods from packages/scaffold-config right into the packages/server/lib/...../reporter.js file and use them as is.

@sfsepark would you be interested in making a PR? I can help with adding a test, if you want to pull the repo down, run yarn, you should be ready to go. You can just add the line and run the project locally against your own projects, eg yarn cypress:open at the root of this repo, choose your project, see if it works.

ernestostifano commented 11 months ago

Hello everyone, is someone currently working on this? If not, I could try following @lmiller1990 suggestions (no promises).

ernestostifano commented 11 months ago

Hi @lmiller1990 I noticed that your commit have been reverted (https://github.com/cypress-io/cypress/pull/26735), so I cannot import your utilities. Will try to replicate your fix by adding the utilities again, but inside the server package.

ernestostifano commented 11 months ago

Hello everyone, I created this draft PR (https://github.com/cypress-io/cypress/pull/28404), but I am not able to try if it works (see the comment I wrote there).

@lmiller1990 would you be able to help writing some tests? I had to turn your utils synchronous because if not the whole process had to become asynchronous and I don't know what impacts it could have elsewhere.

lmiller1990 commented 11 months ago

Hey @ernestostifano - I'm not working on Cypress now, and probably won't be able to prioritize reviewing this one at this point... I'd recommend tagging some contributors who have recently committed to develop to get someone who is more actively working on and reviewing code.

I hope this can get reviewed - Yarn PNP support would be nice. Have you successfully built a Cypress binary from your branch to test it locally? My understanding is the main issues occur once you do a npm install and try running from the build binary with the electron app.

ernestostifano commented 11 months ago

@lmiller1990 thanks for your response! I completely understand. I will try building the binary, sounds like a good idea. Thanks!