cypress-io / cypress

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

Ability to define multiple 'on' events in pluginsFile without overwriting previously defined #5240

Open Alastair-Spencer opened 4 years ago

Alastair-Spencer commented 4 years ago

Current behavior:

The last declaration of the on before hook is being executed using cypress open

Desired behavior:

In this example, i'm expecting both before browser launch's to execute.

Steps to reproduce: (app code and test code)

Add following code to plugins/index.js

module.exports = (on, config) => {
  on('before:browser:launch', (browser = {}, args) => {
    console.log('WONT GET CALLED', args);
    return args;
  });

  on('before:browser:launch', (browser = {}, args) => {
    console.log('WILL GET CALLED', args);
    return args;
  });
};

Versions

v3.4.1, MacOS Mojave, Chrome / Electron

jennifer-shehane commented 4 years ago

@Alastair-Spencer I'm a little unsure on the usecase of having 2 listeners set up for before:browser:launch, the second before:browser:launch is overwriting the initial before:browser:launch. The before:browser:launch will only be called once.

Please explain the usecase for needing 2 listeners. You may also want to utilize some of the following methods to bind or unbind from the events. https://docs.cypress.io/api/events/catalog-of-events.html#Binding-to-Events

Alastair-Spencer commented 4 years ago

@jennifer-shehane

I apologise for not making the issue a little clearer - If I was to have multiple plugins hooking from the same event, this would prevent the first one from running and cause a race condition.

They look odd being placed together in this file but this is where plugins are initialised and will hook off these events being fired. Hope that helps! 👍

liegeandlief commented 4 years ago

To add to this issue, I agree that it would be useful to have multiple listeners. Currently if a plugin I use implements the before:browser:launch hook then it means I cannot also use this hook.

An example is when using one of Cypress' recommended visual regression plugins https://github.com/meinaart/cypress-plugin-snapshots.

The plugin has to be set up as follows:

const { initPlugin } = require('cypress-plugin-snapshots/plugin');

module.exports = (on, config) => {
  initPlugin(on, config);
  return config;
};

In initPlugin the before:browser:launch hook is used.

But in my plugins/index.js file I also want to use this hook in order to set the browser window size argument. So I tried this:

const { initPlugin } = require('cypress-plugin-snapshots/plugin')

module.exports = (on, config) => {
  on('before:browser:launch', (browser = {}, launchOptions) => {
    if (browser.name === 'chrome' || browser.name === 'chromium' || browser.name === 'canary') {
      launchOptions.args.push('--window-size=1200,800')
    }
    return launchOptions
  })

  initPlugin(on, config)
  return config
}

But only the last listener gets called. I have seen this method of plugin initialisation across several visual regression plugins suggesting this is a common way of getting plugin consumers to use the plugin, but it means that any hooks the plugin uses can't then be used by other plugins.

cooleiwhistles commented 3 years ago

We are facing the same issue. We are using the plugin cypress-browser-permissions which modifies the before:browser:launch hook, overwriting the change in our plugins/index.js file to modify window size in before:browser:launch.

amitzur commented 2 years ago

@jennifer-shehane we are also facing the same issue with Applitools' SDK. We rely on the before:run and after:run hooks, and therefore our users cannot listen to these events when using our tool. Having multiple listeners per event is actually the expected behavior from this API, similar to how EventEmitter works in Node.js (it's even called the same name - on). Is this planned to be fixed?

btaluy commented 2 years ago

@jennifer-shehane we are also facing the same issue with Applitools' SDK. We rely on the before:run and after:run hooks, and therefore our users cannot listen to these events when using our tool. Having multiple listeners per event is actually the expected behavior from this API, similar to how EventEmitter works in Node.js (it's even called the same name - on). Is this planned to be fixed?

This is exactly what we are currently facing. Is there any fix planned for this?

luxalpa commented 2 years ago

This just cost me 4 (thankfully paid) hours. Would be nice to get this fixed.

gallayl commented 2 years ago

If I register a listener, I won't expect that it will affect the already registered ones. Right now it feels like an unwanted side effect that I have to work around somehow... I expected something like how NodeJS event listeners works. Also put some futile effort to figure out what's going on :)

on is a function that you will use to register listeners on various events that Cypress exposes.

from Cypress docs

mikstime commented 2 years ago

We are facing the same issue. Here is a cheap workaround for multiple listeners on each event.

const makeChainableListeners = () => {
    const events = {};
    const chainListeners = (action, fn) => {
        if (!events[action]) {
            events[action] = [];
        }
        events[action].push(fn);
    };
    const applyListeners = on => {
        for (const [action, fns] of Object.entries(events)) {
            if (action === 'task') {
                on(action, fns.reduce((a, v) => ({ ...a, ...v }), {}));
            } else {
                on(action, async function(...args) {
                    for (const fn of fns) {
                        await fn.apply(this, args);
                    }
                });
            }
        }
    };
    return [chainListeners, applyListeners];
};

export default async function plugins(on, config) {
    const [chainListeners, applyListeners] = makeChainableListeners();
    chainListeners('after:run', () => console.log('after run 1'));
    chainListeners('after:run', () => console.log('after run 2'));
    initPlugin(chainListeners, config);

    applyListeners(on);

    return config;
};
zawiasam commented 2 years ago

We are facing the same issue. Here is a cheap workaround for multiple listeners on each event.

const makeChainableListeners = () => {
    const events = {};
    const chainListeners = (action, fn) => {
        if (!events[action]) {
            events[action] = [];
        }
        events[action].push(fn);
    };
    const applyListeners = on => {
        for (const [action, fns] of Object.entries(events)) {
            if (action === 'task') {
                on(action, fns.reduce((a, v) => ({ ...a, ...v }), {}));
            } else {
                on(action, async function(...args) {
                    for (const fn of fns) {
                        await fn.apply(this, args);
                    }
                });
            }
        }
    };
    return [chainListeners, applyListeners];
};

export default async function plugins(on, config) {
    const [chainListeners, applyListeners] = makeChainableListeners();
    chainListeners('after:run', () => console.log('after run 1'));
    chainListeners('after:run', () => console.log('after run 2'));
    initPlugin(chainListeners, config);

    applyListeners(on);

    return config;
};

Just keep in mind that this will not work in case when you need to return value like in case of "before:browser:launch" You will have to adjust this code for your needs

brian-mann commented 1 year ago

Yeah we should definitely allow certain (or all) event handlers to be added, but we'd need to define the specification for this behavior. For instance, should the callbacks be called simultaneously, or async sequentially, yielding modified objects in the callback function, etc.

We'd also need to factor in how you potentially "remove" a listener, since we only yield you the on object, effectively we'd be inheriting all of the methods of how event-emitter works.

Once we define the specification and the expected behavior (this would also likely be a breaking change) then we could go ahead and implementation. Definitely in favor of doing this though.

emilyrohrbough commented 1 year ago

related to https://github.com/cypress-io/cypress/issues/22428

francisco-polaco commented 1 year ago

This just made me spend 8 hours :cry: We're using cypress-plugin-snapshots and my before:browser:launch listener wasn't being called.

I believe a lot of people will and already bumped into this.

ext commented 1 year ago

A workaround we hacked together is to wrap the on function with an EventEmitter forwarding all events:

class EventForwarder {
    private emitter: EventEmitter;
    private task: Cypress.Tasks;
    public on: Cypress.PluginEvents;

    public constructor() {
        this.emitter = new EventEmitter();
        this.task = {};
        this.on = (action, arg) => {
            if (action === "task") {
                Object.assign(this.task, arg);
            } else {
                this.emitter.on(action, arg as () => void);
            }
        };
    }

    public forward(on: Cypress.PluginEvents): void {
        for (const event of this.emitter.eventNames()) {
            /* eslint-disable-next-line @typescript-eslint/no-explicit-any */
            on(event as any, (...args: unknown[]) => {
                for (const listener of this.emitter.listeners(event)) {
                    listener(...args);
                }
            });
        }
        on("task", this.task);
    }
}

It can be used as following:

 export default defineConfig({
     e2e: {
-        setupNodeEvents(on, config) {
+        setupNodeEvents(cypressOn, config) {
+            const eventForwarder = new EventForwarder();
+            const on = eventForwarder.on;

             plugin1(on);
             plugin2(on);
             plugin3(on);

             on("before:run", () => {
                 /* ... */
             });
+
+            eventForwarder.forward(cypressOn);
        },
    }
});
Devstored commented 1 year ago

Hi @ext

Can you post an example code please? In my case I use the library: cypress-aiotests-reporter + cypress-mochawesome-reporter.

In the file "cypress.config.js" :

const { EventForwarder } = require("./eventForwarder");
e2e: {
    //setupNodeEvents(on, config) {
    setupNodeEvents(cypressOn, config) {
      const eventForwarder = new EventForwarder();
      const on = eventForwarder.on;
      registerAIOTestsPlugin(on,config);
      require('@cypress/grep/src/plugin')(config)
      require('cypress-mochawesome-reporter/plugin')(on)
      on('before:run', async (details) => {
        console.log('override before:run');
        await beforeRunHook(details);
      });

      on('after:run', async () => {
        console.log('override after:run');
        await afterRunHook();
      });
      eventForwarder.forward(cypressOn);
      on('task', {
        getFiles(path) {
          let list = [];
          return getFiles(path, list)
        },
        log (message) {
          console.log(message)
          return null
        },

      return config
    },
  },

I created an "event Forwarder.ts" file at the root of the project:

// eventForwarder.ts
import { EventEmitter } from "events";

export class EventForwarder {
    private emitter: EventEmitter;
    private task: Cypress.Tasks;
    public on: Cypress.PluginEvents;

    public constructor() {
        this.emitter = new EventEmitter();
        this.task = {};
        this.on = (action, arg) => {
            if (action === "task") {
                Object.assign(this.task, arg);
            } else {
                this.emitter.on(action, arg as () => void);
            }
        };
    }

    public forward(on: Cypress.PluginEvents): void {
        for (const event of this.emitter.eventNames()) {
            /* eslint-disable-next-line @typescript-eslint/no-explicit-any */
            on(event as any, (...args: unknown[]) => {
                for (const listener of this.emitter.listeners(event)) {
                    listener(...args);
                }
            });
        }
        on("task", this.task);
    }
}

I created a "tsconfig.json" file at the root of the project:

    "compilerOptions": {
      "target": "es5",
      "lib": [
        "es5",
        "dom"
      ],
      "types": [
        "node",
        "cypress",
        "cypress-mochawesome-reporter"
      ]
    },
    "include": [
      "**/*.ts"
    ]
  }

The plugins run fine but the content of the .html file for cypress-mochawesome-reporter is empty. I do not understand why ?

Thanks for your help !

elaichenkov commented 1 year ago

Hi all, I've faced the same issue. So, I decided to create a library that can fix the problem. You just need to install the library with the following command:

npm i -D cypress-plugin-init 

And then, you need to import the initPlugins function and use it in your cypress.config.ts file:

import { initPlugins } from 'cypress-plugin-init';

export default defineConfig({
  e2e: {
   // ...
    setupNodeEvents(on, config) {
      // invoke the function with all plugins that you need instead of the 'plugin1' and 'plugin2'
      initPlugins(on, [plugin1, plugin2]);
    },
   // ...
  },
});
Devstored commented 1 year ago

Hi @elaichenkov ,

Thanks for your sharing but not work for me :

initPlugins(on, [registerAIOTestsPlugin(on,config), require('cypress-mochawesome-reporter/plugin')(on)]);

Traceback :



TypeError: plugin is not a function
    at /Users/dino/Documents/tests/node_modules/cypress-plugin-init/index.js:16:9
    at Array.forEach (<anonymous>)
    at initPlugins (/Users/dino/Documents/tests/node_modules/cypress-plugin-init/index.js:15:13)
    at setupNodeEvents (/Users/dino/Documents/tests/cypress.config.js:84:7)
    at /Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/packages/server/lib/plugins/child/run_plugins.js:122:14
    at tryCatcher (/Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/node_modules/bluebird/js/release/util.js:16:23)
    at Function.Promise.attempt.Promise.try (/Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/node_modules/bluebird/js/release/method.js:39:29)
    at RunPlugins.load (/Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/packages/server/lib/plugins/child/run_plugins.js:119:9)
    at RunPlugins.runSetupNodeEvents (/Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/packages/server/lib/plugins/child/run_plugins.js:59:17)
    at EventEmitter.<anonymous> (/Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/packages/server/lib/plugins/child/run_require_async_child.js:185:22)
    at EventEmitter.emit (node:events:513:28)
    at EventEmitter.emit (node:domain:489:12)
    at process.<anonymous> (/Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/packages/server/lib/plugins/util.js:33:22)
    at process.emit (node:events:513:28)
    at process.emit (node:domain:489:12)
    at process.emit.sharedData.processEmitHook.installedValue [as emit] (/Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/node_modules/@cspotcode/source-map-support/source-map-support.js:745:40)
    at emit (node:internal/child_process:937:14)
    at processTicksAndRejections (node:internal/process/task_queues:83:21)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.```
elaichenkov commented 1 year ago

Hi @elaichenkov ,

Thanks for your sharing but not work for me :

initPlugins(on, [registerAIOTestsPlugin(on,config), require('cypress-mochawesome-reporter/plugin')(on)]);

Traceback :


TypeError: plugin is not a function
    at /Users/dino/Documents/tests/node_modules/cypress-plugin-init/index.js:16:9
    at Array.forEach (<anonymous>)
    at initPlugins (/Users/dino/Documents/tests/node_modules/cypress-plugin-init/index.js:15:13)
    at setupNodeEvents (/Users/dino/Documents/tests/cypress.config.js:84:7)
    at /Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/packages/server/lib/plugins/child/run_plugins.js:122:14
    at tryCatcher (/Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/node_modules/bluebird/js/release/util.js:16:23)
    at Function.Promise.attempt.Promise.try (/Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/node_modules/bluebird/js/release/method.js:39:29)
    at RunPlugins.load (/Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/packages/server/lib/plugins/child/run_plugins.js:119:9)
    at RunPlugins.runSetupNodeEvents (/Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/packages/server/lib/plugins/child/run_plugins.js:59:17)
    at EventEmitter.<anonymous> (/Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/packages/server/lib/plugins/child/run_require_async_child.js:185:22)
    at EventEmitter.emit (node:events:513:28)
    at EventEmitter.emit (node:domain:489:12)
    at process.<anonymous> (/Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/packages/server/lib/plugins/util.js:33:22)
    at process.emit (node:events:513:28)
    at process.emit (node:domain:489:12)
    at process.emit.sharedData.processEmitHook.installedValue [as emit] (/Users/dino/Library/Caches/Cypress/12.8.1/Cypress.app/Contents/Resources/app/node_modules/@cspotcode/source-map-support/source-map-support.js:745:40)
    at emit (node:internal/child_process:937:14)
    at processTicksAndRejections (node:internal/process/task_queues:83:21)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.```

Hey @Devstored You don't have to invoke the plugins. Just pass them in the array:

- initPlugins(on, [registerAIOTestsPlugin(on,config), require('cypress-mochawesome-reporter/plugin')(on)]);
+ initPlugins(on, [registerAIOTestsPlugin, require('cypress-mochawesome-reporter/plugin')]);

However, I think that won't work as well. Because, currently, the plugin accepts only one parameter (on). But in your case, registerAIOTestsPlugin expects two parameters. Please, create an issue in the repo if it doesn't work.

Devstored commented 1 year ago

@elaichenkov

Thanks, but i tried and same issue : `An error was thrown in your plugins file while executing the handler for the before:run event.

The error we received was:

TypeError: Cannot convert undefined or null to object`

elaichenkov commented 1 year ago

@Devstored Please, update the plugin to the 0.0.7 version and try again:

import { initPlugins } from 'cypress-plugin-init';

export default defineConfig({
  e2e: {
    // ...
    setupNodeEvents(on, config) {
      initPlugins(on, [registerAIOTestsPlugin, require('cypress-mochawesome-reporter/plugin')], config);
    },
    // ...
  },
});
Devstored commented 1 year ago

Hi @elaichenkov, It works perfectly!

Thanks for your help

skitterm commented 9 months ago

I wanted to add my use-case as a plugin developer:

My Cypress plugin makes use of multiple events (before:run and task). Currently, I either have to have an install command (that will override or be overridden by any events of the same name in the users' setupNodeEvents) or the user has to import and individually call my functions for each event, which is cumbersome for them.

Just wanted to advocate for merging events automatically in Cypress, otherwise users with multiple plugins or custom events of their own will be out of luck.