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

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

Open
massimeddu-sonic opened this issue Nov 15, 2021 · 18 comments
Open

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

massimeddu-sonic opened this issue Nov 15, 2021 · 18 comments
Labels
E2E Issue related to end-to-end testing Triaged Issue has been routed to backlog. This is not a commitment to have it prioritized by the team.

Comments

@massimeddu-sonic
Copy link

massimeddu-sonic commented Nov 15, 2021

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
Copy link
Contributor

BlueWinds commented Nov 16, 2021

@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
Copy link
Contributor

flotwig commented Nov 17, 2021

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:

try {
p = path.resolve(projectRoot, reporterName)
// try local
debug('trying to require local reporter with path:', p)
// using path.resolve() here so we can just pass an
// absolute path as the reporterName which avoids
// joining projectRoot unnecessarily
return require(p)
} catch (err) {
if (err.code !== 'MODULE_NOT_FOUND') {
// bail early if the error wasn't MODULE_NOT_FOUND
// because that means theres something actually wrong
// with the found reporter
throw err
}
p = path.resolve(projectRoot, 'node_modules', reporterName)
// try npm. if this fails, we're out of options, so let it throw
debug('trying to require local reporter with path:', p)
return require(p)
}

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
Copy link
Author

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
Copy link
Author

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

massimeddu-sonic added a commit to massimeddu-sonic/cypress that referenced this issue Dec 3, 2021
massimeddu-sonic added a commit to massimeddu-sonic/cypress that referenced this issue Dec 6, 2021
massimeddu-sonic added a commit to massimeddu-sonic/cypress that referenced this issue Dec 6, 2021
@cypress-bot cypress-bot bot added stage: work in progress and removed stage: needs review The PR code is done & tested, needs review stage: work in progress labels Dec 7, 2021
@lexanth
Copy link

lexanth commented Feb 2, 2022

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
Copy link

kubijo commented Mar 23, 2022

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
Copy link

kubijo commented Aug 26, 2022

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

@ImSaSu
Copy link

ImSaSu commented Aug 29, 2022

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
Copy link

kubijo commented Aug 29, 2022

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
Copy link

Arman92 commented Oct 4, 2022

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
Copy link

public commented Nov 18, 2022

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
Copy link
Contributor

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.

#26452

@lmiller1990
Copy link
Contributor

lmiller1990 commented Apr 19, 2023

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.

@nagash77 nagash77 added help wanted E2E Issue related to end-to-end testing Triaged Issue has been routed to backlog. This is not a commitment to have it prioritized by the team. labels Apr 20, 2023
@jennifer-shehane jennifer-shehane added good first issue Good for newcomers and removed help wanted labels Oct 4, 2023
@ernestostifano
Copy link

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

@ernestostifano
Copy link

Hi @lmiller1990 I noticed that your commit have been reverted (#26735), so I cannot import your utilities. Will try to replicate your fix by adding the utilities again, but inside the server package.

@ernestostifano
Copy link

Hello everyone, I created this draft PR (#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
Copy link
Contributor

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E2E Issue related to end-to-end testing Triaged Issue has been routed to backlog. This is not a commitment to have it prioritized by the team.
Projects
None yet