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

Mochify Rewrite #229

Closed
mantoni opened this issue Jul 4, 2021 · 14 comments
Closed

Mochify Rewrite #229

mantoni opened this issue Jul 4, 2021 · 14 comments

Comments

@mantoni
Copy link
Owner

mantoni commented Jul 4, 2021

Let's rewrite mochify 🚀

The rewrite branch is an attempt to rethink mochify from the ground up.

Modular setup to only install what is needed

Mochify is currently rather bloated and loads all sorts of dependencies that most projects might never use. We can use npm workspaces to keep everything in one repository and publish smaller modules. I reserved the @mochify npm namespace.

The rewrite introduces "drivers" which are modules that bridge between the mochify CLI and runtime environments to run tests. Here are some ideas for drivers:

Externalise the bundler

Allow to configure a bundle script to run using execa (https://www.npmjs.com/package/execa), similar to what lint-staged does. This eliminates the dependency on browserify and allows to use mochify with other bundlers 🙌. And it removes a lot of the current complexity. 👍

Instead of bundling mocha with the test files, the pre-built mocha/mocha.js bundle is loaded, then a Mochify "client" is loaded, and then the actual script is loaded separately (to retain the correct line numbers for source-maps).

Only the actually passed files need to be bundled. mocha/mocha.js and the Mochify client.js files don't have any dependencies.

Untangle reporter, console, runner and coverage output

Mochify currently does a lot of output parsing. The desired reporter is installed in the browser and the output has to be separated from console.log statements and output from puppeteer or nyc.

Instead, use a custom reporter in the browser and pass ndjson over a communication channel to feed events into a Mocha reporter on the CLI side. This removes the need for excessive parsing since event names can be used to communicate different kinds of output.

Support config files

E.g. cosmiconfig https://www.npmjs.com/package/cosmiconfig


Help wanted

The rewrite branch is a baiscally a runnable exampe of the above ideas, with no unit tests, documentation and a rough implementation. Please feel free to contribute ideas, PRs, ...

@m90 @mroderick @albertyw @fatso83

@m90
Copy link
Collaborator

m90 commented Jul 4, 2021

Woohoo 🎉 this seems to be an amazing start. I love how much simpler this is.

Some uninformed feedback after a first glance:

Zero config

The reason why I started using Mochify in ~2014 was that "it just worked ™️". I would install the package, it picked up my browserify config all by itself and it could run my tests in a real browser 💌. Compared to what was out there at the time this was truly amazing. Maybe I am biased because I still mostly refuse to abandon Browserify in 2021, but I think it's still amazing today: I get a test runner that does exactly what I want with zero config and by installing a single package only.

Playing the devil's advocate, seeing that I now have to install multiple packages and possibly provide a config file makes me feel slightly uneasy. Will zero config work too? Will there be a single package that wraps the CLI and a default driver?

Passing options to Drivers

Maybe you're still planning to do this, but it seems to me as if we'd need to be able to pass options to drivers at some point, which currently seems to be unsupported:

const driver_promise = mochifyDriver();
Imagine one would implement a playwright driver and wanted to tell playwright which engine to use, how would that work?

Custom non-@mochify drivers

Drivers seem to require to reside under the @mochify scope. Is this intentional or should there be a fallback allowing consumers to provide arbitrary drivers?

Default driver

I don't know if you ever had a look at that branch, but I ported Mochify to use playwright instead of puppeteer a while ago, which was relatively painless (but I never got around to know how the testing story should work, so I shamefully abandoned it): https://github.com/mantoni/mochify.js/tree/playwright-firefox. Considering this also adds Firefox and Webkit support I wonder if this should maybe become the default driver instead of puppeteer? Or is puppeteer a nicer option because it's more lightweight? Really not sure.

Extra deps

This might be very subjective because my $DAYJOB consists of googling for error messages of npm failing on humongous dependency trees ~70% of the time (no exaggeration unfortunately), but I am wondering if adding execa and cosmiconfig is really needed. Could this just use child_process? Could config live in package.json on the mochify key?


If time allows I think I'd like to try adding a playwright driver and also see if I can get this working with esbuild instead of Browserify next week. I'll keep you posted on the progress ✌️

@mantoni
Copy link
Owner Author

mantoni commented Jul 4, 2021

Thank you for the quick feedback. Here are my thoughts on your points:

Zero config

This was my design goal with mochify at the time, but you're sort of making the point that we cannot keep it this way yourself 😄. You might want to bundle with esbuild or run tests with playwright and a pluggable design makes this possible. Having a default bundler and driver would require adding them as dependencies which makes it a rather large install again. Especially with puppeteer, which downloads chromium, and browserify that comes with quite a tree of dependencies.

Passing options to Drivers

Yes, we'd need to be able to pass options to drivers. The current state is just a proof of concept. There's a lot to do.

Custom non-@mochify drivers

Yep, it would be nice to allow "external" drivers. The mochify cli could map "puppeteer" and "selenium" to the @mochify names and, if none match, use the configured driver name as is.

Default driver

I had a brief look at your work on playwright, but didn't get to really experiment with it myself. Like with the "Zero config" argument, it might be a great default for you, but other projects might prefer puppeteer for some reason, or even only need a selenium driver. So I'd rather not add a default driver at all.

Extra deps

Sure, we could do the work ourselves. However, execa does a great job and I'd like to keep things simple. I basically stole the execa and string-argv combo from lint-staged and love how easy it was to get it working.


Another note: With the latest commits, I managed to run the sinon.js test suite like this:

../mochify.js/cli/index.js './test/**/*-test.js' --bundle 'browserify --no-detect-globals --plugin [ proxyquire-universal ] --debug' -R dot

@m90
Copy link
Collaborator

m90 commented Jul 5, 2021

A few more words about the "default": I probably could have made this a lot clearer, but when I said "default" I was mostly referring to what is being mentioned in the above-the-fold installation instructions in the README, which of course isn't a bundled default, but I'd argue it creates an implicit default. Right now, it reads:

npm i @mochify/cli @mochify/driver-puppeteer -D

and I would assume this means that 98% people trying it out will install this driver and 90% of users will stick to it, no matter how many other options there are. I understand it's meant to be pluggable, but many consumers won't have time and/or knowledge to make an educated decision about which driver to use, so they will stick to what they initially installed.

So what i was trying to say is: it's a really important decision which driver is mentioned in the npm i command, and being aware of the consequences will pay off.


Another question unrelated to this came to mind: how do you plan to handle versioning in the monorepo? Are drivers and the cli planned to use the same major always or could they theoretically differ and each package has its own set of compatibility guarantees?

@mantoni
Copy link
Owner Author

mantoni commented Jul 5, 2021

it's a really important decision which driver is mentioned in the npm i command

That's a good point. We should make "the best" driver the default. If playwright gives us support for three different browsers, then I agree that it should be the one in the install instructions.

On top of the "default" npm i command, we could come up with documentation for different use cases and corrsponding install / config instructions.

how do you plan to handle versioning in the monorepo?

With npm workspaces, each of the workspace folders is a separate module that can be versioned and published. E.g. npm version patch -w cli would make a new version of @mochify/cli only. The cli has a regular dependency on @mochify/mochify. For the drivers, we should declare a peerDependency on @mochify/mochify to state compatibility. I have successfully used this approach, also with multiple major versions:

{
  "peerDependencies": {
    "@mochify/mochify": "^1 || ^2"
  }
}

@m90
Copy link
Collaborator

m90 commented Jul 6, 2021

Another random shower thought of mine:

Assuming we want to completely offload the bundling to the consumer and just have Mochify consume a bundle bundled by basically everything, why doesn't this command:

mochify './test/**/*-test.js' --bundle 'browserify --no-detect-globals --plugin [ proxyquire-universal ] --debug' -R dot

look like this instead, freeing us of having to handle the bundling command:

browserify --no-detect-globals --plugin [ proxyquire-universal ] --debug' -R dot './test/**/*-test.js' | mochify

i.e. Mochify just reads the bundled files from stdin and that's it? Is there anything we couldn't do like this? Is this complicated because of Windows (not that I have any idea about this)?

@mantoni
Copy link
Owner Author

mantoni commented Jul 6, 2021

We could support your proposal, to read from stdin. I'm generally a friend of a UNIX style interface. I would still support a bundle option, especially for config files. Those lines with pipes tend to get really long and hard to understand.

@m90
Copy link
Collaborator

m90 commented Jul 6, 2021

I would still support a bundle option, especially for config files.

Ah, so the bundle command would go in the config file in a "proper" setup and wouldn't be passed on the command line at all?

@mantoni
Copy link
Owner Author

mantoni commented Jul 6, 2021

That's the idea so far. We can allow to specify / override it on the command line though.

@m90
Copy link
Collaborator

m90 commented Jul 6, 2021

I found half an hour to play around with this and I couldn't find any major issues:

  • adding a playwright driver was very simple: Add playwright driver #230
  • using esbuild worked out of the box
  • I ran the test suites of 2 smaller projects (~150 tests each) that are currently using Mochify 8 just fine using the rewrite branch

@m90
Copy link
Collaborator

m90 commented Jul 6, 2021

About reading the bundle from stdin as an option instead of passing a command: I looked into how this could work on a code level and found it slightly odd that bundling happens on library level instead of CLI level. Is there any reason you don't want to just pass a plain bundle string to the library? When used as a library in code (gulp or whatever), I would assume consumers would also want to programatically use their bundler(s) and could just pass a string or a stream that emits the bundle?

@mantoni
Copy link
Owner Author

mantoni commented Jul 6, 2021

Hm, interesting point. My design goal here would be that the API and the CLI work the same way. Basically all options are a one-to-one mapping of configs that can be passed to mochify(config). Ideally calling mochify() and npx @mochify/cli does the same thing. That's why I've put the bundling into the API.

However, the API can do more than the CLI and allow the bundle to be a stream. What do you think?

@m90
Copy link
Collaborator

m90 commented Jul 7, 2021

Ideally calling mochify() and npx @mochify/cli does the same thing.

This makes a lot of sense considering the config file plan. Adding extra options to the API will still work.

@m90
Copy link
Collaborator

m90 commented Jul 8, 2021

For what it's worth I tried implementing a jsdom driver (which would be nice as a very minimal option) over here: https://github.com/mantoni/mochify.js/tree/jsdom-driver

I'm running into something I don't fully understand though:

  • jsdom evaluates all of the passed scripts just fine
  • console.log calls originating from within Mocha will be swallowed and appear nowhere, thus the runner will hang polling for events infinitely
  • when I sprinkle the client with random console.log calls (before and after overwriting the methods to point at write) these will work as expected (i.e. log or create an event that will then be retrieved on polling)

I remember Mocha is doing some custom console modifications, but I cannot really connect the dots here yet.


Some more debugging records here:

  • the logging behavior seems to be a red herring
  • the bundled tests get wrapped in a script tag and are injected into the DOM correctly, which will also execute the contents immediately
  • Mocha collects suites and tests correctly
  • calling mocha.run will never run any tests. The runner instance is returned correctly and doesn't really look different than when using a different driver. Nothing throws.

Found out what is going on here and fixed it in #232

@mantoni
Copy link
Owner Author

mantoni commented Sep 6, 2021

Since we have a first version of the rewrite published, I think it's time to have more focused discussions on the individual issues. I have created a Milestone to group open issues here: https://github.com/mantoni/mochify.js/milestone/1

Please feel free to add whatever you feel is missing. Thanks a lot for all the help!

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

2 participants