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

Plugins API - preprocessing with source file - electron/ chrome run #1654

Closed
paulpruteanu opened this issue May 1, 2018 · 19 comments
Closed

Comments

@paulpruteanu
Copy link

paulpruteanu commented May 1, 2018

Current behavior:

Not being able to run "cypress run" (default to electron), when preprocessing the source file.
Ending up in "Uncaught ReferenceError: require is not defined"

Desired behavior:

To be able to run it successfully.

Steps to reproduce:

# cypress/plugins/index.js
module.exports = (on) => {
  on('file:preprocessor', (file) => Promise.resolve(file.filePath))
  /* "cypress run" (default to electron) throws this error: Uncaught ReferenceError: require is not defined
  cypress run --browser chrome behaves the same
  it works resolving file.outputPath, but it's not good enough for staged containers that doesn't have the outputPath
  at the time */
}

screen shot 2018-05-01 at 13 04 45

same thing goes for

const watch = require('@cypress/watch-preprocessor')

module.exports = (on) => {
  on('file:preprocessor', watch())
}

Versions

cypress@2.1.0, MacOs Sierra 10.12.6

@paulpruteanu paulpruteanu changed the title Plugins API - preprocessing with source file - electron run Plugins API - preprocessing with source file - electron/ chrome run May 1, 2018
@jennifer-shehane jennifer-shehane added the stage: needs investigating Someone from Cypress needs to look at this label May 1, 2018
@chrisbreiding
Copy link
Contributor

By configuring the preprocessor in either of those ways, you're bypassing the default preprocessing and bundling that Cypress does. That means you can't use any features that aren't supported by the browser you're running the tests in. In this case, it looks like there's a commonjs require() in your code or code you import, which isn't supported by any browser. If you use the default preprocessor, it will handle converting any require()s into browser-compatible code.

Is there a reason you can't use the default preprocessor?

@jennifer-shehane jennifer-shehane added stage: awaiting response Potential fix was proposed; awaiting response and removed stage: needs investigating Someone from Cypress needs to look at this labels May 1, 2018
@paulpruteanu
Copy link
Author

paulpruteanu commented May 1, 2018

@chrisbreiding yes. My requirement is to selectively pre-process some of the tests (not all). So instead of browserifying everything just to make it compatible with the browser, isn't any other way?
Browserifying them all (140 test files) ends up in the following error:
screen shot 2018-05-01 at 16 10 10

If I trim the files down to tens, everything works fine.

@chrisbreiding
Copy link
Contributor

chrisbreiding commented May 1, 2018

You need to browserify anything that's not compatible with the browser. There's no getting around that. You can certainly do that selectively, but it will require a little bit of logic. Something like:

@chrisbreiding
Copy link
Contributor

Sorry, that wasn't complete before I submitted it.

const browserify = require('@cypress/browserify-preprocessor')
const watch = require('@cypress/watch-preprocessor')

module.exports = (on) => {
  on('file:preprocessor', (file) => {
    if (/* file doesn't need preprocessing */) {
      return watch()(file)
    } else {
      return browserify()(file)
    }
  })
}

@paulpruteanu
Copy link
Author

paulpruteanu commented May 1, 2018

@chrisbreiding watch()(file) resolves source filePath, which I mentioned already to be failing, for the reasons you explained. Ideally there should be a way to exit the callback without resolving anything, which would let the normal preprocessor kick through.

@chrisbreiding
Copy link
Contributor

Please see my above comment. The one above that had incomplete code.

@paulpruteanu
Copy link
Author

@chrisbreiding the watcher returns filePath (source file), which gets us back to square one.

    const filePath = file.filePath

    // if shouldWatch is false, this is a one-time run, probably in CI
    // so we don't need to watch
    // since this function can get called multiple times with the same
    // filePath, we return if we already have a cached watcher
    // since we don't want or need to re-initiate a watcher for it
    if (!file.shouldWatch || watchers[filePath]) {
      return filePath
    }

@chrisbreiding
Copy link
Contributor

If you need to resolve a different file path, you can do so like this:

return watch()(file).then(() => '/desired/output/path')

@paulpruteanu
Copy link
Author

There's no output path given I start from an empty install. No output files have been processed regularly at that time.

@chrisbreiding
Copy link
Contributor

How are your files being processed in the case where you don't want them browserified?

@paulpruteanu
Copy link
Author

Nope. For the ones that does not need processing, they should flow into the default native pre-processor.

@chrisbreiding
Copy link
Contributor

There's no output path given I start from an empty install. No output files have been processed regularly at that time.

What does it mean for those files to be 'processed regularly'?

@paulpruteanu
Copy link
Author

paulpruteanu commented May 1, 2018

They get transpiled by the default preprocessor, outside the Plugins API, right? But I don't have this option with the current implementation.

@chrisbreiding
Copy link
Contributor

If that's the case, then why is there no output path? Also, can you explain why some files do not need to be or cannot to be processed?

@paulpruteanu
Copy link
Author

paulpruteanu commented May 1, 2018

On the first install, you won't have an output directory eg /Library/Application\ Support/Cypress/cy/production/projects/cypress-example-todomvc-a60a329e6511b7eefa5286529d267046/bundles/cypress/integration/app_spec.js, therefore you can't do Promise.resolve(file.outputPath), because there's nothing there processed already.
You can't resolve it to file.filePath either, as this would get me in trouble with the browser as it won't understand it.

Worth mentioning that outputPath will be created, if I don't use on('file:preprocessor') at all, as the native cypress bundling would kick in.

The business reason for selectively preprocessing files is the feature toggles concept I'm following. Given a project build with certain toggles in place, I need a way to decide which tests needs to be skipped (replace "it" with "xit"), provided the covered features aren't part of the build (toggled-off).

@chrisbreiding
Copy link
Contributor

chrisbreiding commented May 1, 2018

I see. In that case, is it alright if they don't run instead of being skipped with xit? If so, the following should work:

  • Create an 'empty' test file with the contents it('skipped feature')
  • Conditionally Promise.resolve the path to that file for any test file that needs to be skipped

That way you're resolving a file that exists while skipping tests you don't want run.

In 3.0, which is being released relatively soon, there will be support for spec globbing, which may also work for that purpose. Given you name or structure your files in a certain way, you could use a glob to only include the files you want run. You could do that instead of the above solution once 3.0 is out.

@brian-mann
Copy link
Member

This sounds like an elongated XY problem that is finally realized at the end. If you're trying to skip tests conditionally there is a much better way to do that.

You could simply wrap describe blocks into a conditional, or handle this in a beforeEach by parsing the test title and running this.skip() inside of the test.

@paulpruteanu
Copy link
Author

paulpruteanu commented May 1, 2018

@chrisbreiding my requirement is to still be able to run other "its" within the file, so this won't work.
@brian-mann I get your point, but this would mix the toggling with testing concern, I'd rather keep it separated. With the current implementation, I only need to amend a mapping.json which links certain test.js files to certain its within, and leave it to a pre-processor to transform the files accordingly.

My only blocker is that it does it for all 140 files, instead of just 2-3 files which are actually subject to having certain its being skipped.

@paulpruteanu
Copy link
Author

process.setMaxListeners(0); fixed it for me. Ref on issue here.

@jennifer-shehane jennifer-shehane removed stage: awaiting response Potential fix was proposed; awaiting response labels Jan 16, 2019
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

No branches or pull requests

4 participants