Closed mantoni closed 3 years ago
Woohoo :tada: this seems to be an amazing start. I love how much simpler this is.
Some uninformed feedback after a first glance:
The reason why I started using Mochify in ~2014 was that "it just worked :tm:". I would install the package, it picked up my browserify
config all by itself and it could run my tests in a real browser :love_letter:. 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?
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: https://github.com/mantoni/mochify.js/blob/764b1349d52ec8fdf7ce7e4d7bf6863086b0524a/mochify/index.js#L16 Imagine one would implement a playwright
driver and wanted to tell playwright
which engine to use, how would that work?
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?
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.
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 :v:
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
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?
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"
}
}
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)?
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.
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?
That's the idea so far. We can allow to specify / override it on the command line though.
I found half an hour to play around with this and I couldn't find any major issues:
esbuild
worked out of the boxAbout 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?
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?
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.
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:
I remember Mocha is doing some custom console modifications, but I cannot really connect the dots here yet.
Some more debugging records here:
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
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!
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:
driver-puppeteer
(first implementation exists)driver-selenium
to use the selenium protocol (exmin-wd
)driver-chromedriver
could be a bridge for https://www.npmjs.com/package/chromedriverExternalise the bundler
Allow to configure a
bundle
script to run usingexeca
(https://www.npmjs.com/package/execa), similar to whatlint-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 Mochifyclient.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 frompuppeteer
ornyc
.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/cosmiconfigHelp 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