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

Added ability to pass a reporter module to mocha-multi instead of limiting to only strings #97

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jamesmortensen
Copy link

When running programmatically with mocha-multi, we're limited to passing in strings, and then mocha-multi and Mocha require those reporters using "require". This creates challenges for reporters built using ESM. Many other test frameworks deal with this by allowing the reporter to be required/imported in a launcher script and then passing the imported module into the framework instead.

Mocha has this implemented already, but mocha-multi does not.

I added the ability to pass in a reporter module into mocha-multi. Prior to making the updates, I tried several things, including transpiling the reporter to CJS. At the end of the day, what solved my problem was modifying this module so that neither Mocha or mocha-multi is responsible for importing the reporter.

I added the following items to help validate functionality:

  • New test cases written in ESM.
  • An example custom reporter written in ESM
  • A launcher script that imports the ESM reporter and passes the module, with some options, into mocha-multi as reporter options.
  • New verification file to specifically deal with this use case.
  • Updated README with instructions on how to use it.

Hoping this helps others as well!

@glenjamin
Copy link
Owner

do you have any links to the related mocha functionality and documentation?

It seems to me like it would be far easier to support this the other way around - to have a small shim CommonJS module which loads the .mjs version and re-exports it - does that work?

@jamesmortensen
Copy link
Author

Hi @glenjamin originally I tried to see if I could re-export, but I kept running in circles trying to make everything work together. I think one problem I had is my reporter needed to import other esm modules, like node-fetch, and mocha just kept choking on it when trying to require from within. Even transpiling to cjs with Babel or TypeScript wouldn't work. I don't think I could begin to list all of the complications and dead ends I went down while trying to figure this out.

If the reporter is imported before being passed to mocha, then there are no incompatibility issues.

Here is the answer to your questions about the Mocha documentation and existing functionality.
The mocha documentation for mocha.reporter shows that the reporterName argument can be a string or a function constructor.

Name Type Attributes Description
reporterName String | function   Reporter name or constructor.

And in the source code on line 322 in mocha.js, we see that it first checks the type of reporterName. If it is a function, we directly store it as the reporter class. If it is a string, then mocha attempts to use require to import it.

https://mochajs.org/api/mocha.js.html#line322

I think passing in a function or constructor into the mocha-multi reporterOptions builds on top of the existing mocha functionality and makes it easier to inject different kinds of reporters.

Jest and I believe maybe jasmine also make it easier to pass in reporter constructors and do multiple reporters by default. I think this is one of the reasons this pull request helps make Mocha more competitive in this space.

Please let me know if I can provide more clarity or any more details, and thank you for starting the review process before the weekend. :)

@glenjamin
Copy link
Owner

Oh, I meant mocha docs for making things work with ESM code!

@jamesmortensen
Copy link
Author

To the best of my knowledge, there are no docs on Mocha for making it work with ESM.

There are various blog posts which I used to stitch this together. I have a demo here: https://github.com/jamesmortensen/mocha-esm-loader-demo and a library that loads tests written in ESM: https://github.com/jamesmortensen/mocha-esm-loader.

Please let me know of I can provide any more details.

@jamesmortensen
Copy link
Author

Hi @glenjamin I wanted to check in on this pull request. Is there anything else you need from me in order to validate and get this change merged? Thank you!

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 this pull request may close these issues.

None yet

2 participants