Export globally mocha and runner instances

[cgalvarez]

First of all, thank you very much for your efforts and making this awesome package open source!

I'm using it to get code coverage for my Atom package, and I need the mocha and runner instances available to hook my coverage collector into them.

As you can see, the changes are minimal and doesn't change any default functionality.

Here you can read about those hooks I need. Summarizing:

• Runner events, which I can subscribe like global.mocha.runner.on('end', cb) with this PR.
• Mocha instance hooks, which I can hook into like global.mocha.instance.beforeAll(cb) with this PR.

Hope you found it useful and merge it to this awesome package.

I'm here for any improvements/corrections/issues you may have!

[Alhadis]

No, thank you for the kind words. 😀 I'm genuinely flattered knowing this test-runner's helping other people in their projects too. 😉

Regarding "coverage"... that's not something I have much knowledge in, and I've been interested in reading up on it more to learn how it works. So if this gets atom-mocha closer to doing whatever Istanbul does, the better. ;)

[Alhadis]

Alright mate, it looks good to me. 😀 Just some questions. In particular, I'd like to ask about the necessity to have globalise a Mocha object (which might confuse me later on, trying to tell which Mocha is which).

Is this the API you'd need to hack into to track test coverage? =) If not, I'm happy to sit down and add more formal support for coverage generation.

[cgalvarez]

Regarding "coverage"... that's not something I have much knowledge in, and I've been interested in reading up on it more to learn how it works.

I'll try to give you a mini briefing about code coverage...

1. You write your specs for your project.
2. You run your specs with a test runner (i.e. mocha), so you can test if your code does what you expect it to do.

Using a coverage tool needs two more steps: one intermediate and another at the end. The coverage tool needs to instrument your source code (the one being tested, not your specs). In my case, istanbul/nyc provides you info about:

• Statements covered (which code actions have been executed, and how many times).
• Branches covered (which branches have been taken/entered, and how many times; think of if/else branches or ternary operators).
• Functions covered (which functions have been invoked by your specs, and how many times).
• Lines covered (which lines have been executed by your specs, and how many times; please, note that this is different from statements, since one line can contain multiple statements!).

Thus, the coverage tool needs to transform (instrument) your source code, so it can collect the previous info accurately. Note that these instrumented files are different from your original source code.

Once you have instrumented your source files, then it's time to run your specs requiring/using those instrumented files, so when your test-runner (mocha through your atom-mocha package) runs your specs, it executes the instrumented code and it starts collecting the coverage info stated above, which is invaluable (at least to me) to know which pieces of your source code are not being tested with your specs (so they could lead to some potential bugs, since you haven't tested them).

istanbul/nyc collects that info in global.__coverage__ object. Being so, I need a way to retrieve it and save into a physical file to feed nyc later and generate reports to analyze the coverage info (because reading that info in its raw format is a pain in the ass!).

global.__coverage__ is modified by each spec run by the mocha runner. Here we can confuse with the term runner:

1. atom-mocha is the test runner for Atom.
2. mocha has its own runner for your specs, written with the assertion libraries of your choice (expect, should, chai,...).

So I can start now answering your questions...

In particular, I'd like to ask about the necessity to have globalise a Mocha object (which might confuse me later on, trying to tell which Mocha is which).

I need (2), which is the returned value from invoking mocha.run() here, being mocha the instance returned by invoking const mocha = new Mocha(...). The mocha runner (2) provides multiple events I can subscribe to. Since the runner may execute multiple suites (each one having their own test cases) inside a given file, and run for multiple files, the most performant way to collect that coverage info is when the runner finishes (writing the full coverage info only once to the filesystem), and I do so by subscribing with global.mocha.runner.on('end', collectCoverage) (leveraging my PR). I have found no other way to get a reference to the current, instantiated, running mocha runner but saving its reference when it's returned by mocha.run(...) (and exposing it globally).

That's how the coverage info can be collected when the mocha runner ends, but... I need a reliable way to subscribe at the very beginning, when the runner starts. That's why I expose the mocha instance globally too. I can do that by hooking a callback that it's run (supposedly once, as I've checked locally) at the very beginning (even before instantiating the mocha runner) with global.mocha.instance.beforeAll(cbSubscribeToRunnerEnd).

So the final workflow could be as follow:

1. You instrumentate your source code to generate coverage info.
2. You write your specs for your project and prepare them to use the instrumented files (not your source code).
3. You hook a callback at the very beginning by global.mocha.instance.beforeAll(...), before atom-mocha starts running your specs. That callback (when executed at the very beginning) will subscribe to mocha runner 'end' event by global.mocha.runner.on('end',...), so you collect the coverage info when the runner ends only once.
4. You run your specs with atom-mocha, so you can test if your code does what you expect it to do.
5. After the mocha runner ends, you save the coverage info to the filesystem due to the 'end' event being triggered.
6. Generate any report you want from the saved coverage info by feeding istanbul/nyc with the saved file.

I think the confusion comes from your assignment of the mocha instance to the member AtomMocha.runner here. That's not the runner itself, but the mocha instance. The runner itself is the one returned by AtomMocha.runner.run(...) here.

Anyway, I've just found a way to get the coverage without exporting any globals, but I think it would be useful to provide those globals of this PR to allow developers to customize the behavior of their specs through the mocha instance hook methods (mocha.{before|beforeAll|after|afterAll}()) and its runner events (runner.on('{start|end|suite|suite end|test|test end|hook|hook end|pass|fail}',...)).

I apologize for the long explanation, but I think it's the best way I can explain what I was trying to do, and hope it helps you understanding how code coverage works, because I found no short-guide like this nowhere when I started.

[Alhadis]

and hope it helps you understanding how code coverage works, because I found no short-guide like this nowhere when I started.

Damn. Thanks so much for the comprehensive write-up! 😀 I really appreciate the time you put in to offer me a clear explanation. Seriously.

Anyway, I've just found a way to get the coverage without exporting any globals

How did you achieve that?

What we should be doing (I think) is globalising the AtomMocha instance upon creation, and store the Mocha runner on the AtomMocha singleton once .run() is called. We then rename the old .runner to just .mocha (not ._mocha, no need for the underscore).

That should be enough to cover our coverage needs, I should hope? ;)

Just an FYI: this module's entire API is due for complete overhaul. Feel free to offer your thoughts.

[cgalvarez]

Damn. Thanks so much for the comprehensive write-up! grinning I really appreciate the time you put in to offer me a clear explanation. Seriously.

You're welcome!

How did you achieve that?

Well, summarizing it: now I feed nyc with my test command. nyc is ready to wrap some test runners, and I tried with Atom, and guess... it worked! I'm solving some bugs with nyc right now to make it work with one single command (with nyc instrumenting on the fly, which currently is troubleshooting me).

I will share you the github repo URL of my package (which is a fork of atom-project-manager) after I push all my changes, if you want to try by yourself 😉

What we should be doing (I think) is globalising the AtomMocha instance upon creation, and store the Mocha runner on the AtomMocha singleton once .run() is called. We then rename the old .runner to just .mocha (not ._mocha, no need for the underscore).

That would be a great solution indeed. I noticed that global.AtomMocha was available, but I thought it was the constructor (class), not a singleton. I would offer both (.mocha being the Mocha instance, and .runner being the runner for .mocha, because there is no instance method to get it from .mocha AFAIK), both retrievable through their own getters 😄

Later I'll make the changes and test them. If succeed, will push them and rebase 😉

Just an FYI: this module's entire API is due for complete overhaul. Feel free to offer your thoughts.

I saw it early in the morning, but didn't give it a read. For sure I will offer you any thought I will come up with.

[Alhadis]

Later I'll make the changes and test them. If succeed, will push them and rebase

Excellent, thanks again! ;D

I noticed that global.AtomMocha was available, but I thought it was the constructor (class), not a singleton

It's actually this, I think: 😕 Nowhere else in the code is AtomMocha globalised, so whatever you saw in global scope had to have been one of many bizarre hacks needed to implement this. That'll be simple to fix: give it some other name instead. 😁

[cgalvarez]

I've just checked that nyc was not collecting my coverage as I thought, so I definitely need this PR (my package-lock.json was fetching atom-mocha from my PR branch, not the one officially published on NPM, and that led me to draw erroneous conclusions).

Tomorrow I'll rewrite the PR. I'm exhausted by today 😉

[cgalvarez]

@Alhadis Just commited the changes and rebased to have only one commit to review. I've just checked locally that it works! Waiting for your review comments 😉

[Alhadis]

Thanks so much! 😀 You've been both patient and helpful.

I'm also going to drop that ugly code-injection hack I used for implementing the "open filenames by clicking their names in stack traces" feature. Atom have since added a proper API for doing this sort of thing.

[cgalvarez]

Welcome! FYI, I've been reading your open issue, but I feel still a bit poor-skilled on Atom issues to offer you some useful thoughts.

I'm just deep learning to code Atom packages, so hopefully I will lend you a hand to improve this package (if you want) after acquiring some more skills on this topics 😉

[Alhadis]

Honestly, when I finally get around to rehauling and properly documenting the package's APIs, your opinion on what feels right and what feels weird will be most valuable. 😀 This project started because I needed a functional, responsive, non-frustrating interface for running package tests as I worked on file-icons v2.

Since those features involved dangerous, risky hacks of core package code, I needed to make sure test-coverage was airtight. Most importantly, I needed to see what went wrong when a test failed. Because the test-runner was such a damn success, I wanted others to use and enjoy it too. But I haven't exactly promoted it or anything... so I've not had many users or much feedback to go by ;)

BTW, haven't cut a release yet because I'm wrapping up a PR. Will do so soon, don't worry. 😉

Unrelated trivia:

• autoIt was a reference to a language. It sounded punny at the time, now it just looks stupid.
• I'm the only dev I know stupid enough to base64-encode a meme inside a fucking commit message.

[jccguimaraes]

I am happy to bring any feedback on this. I am very pleased with the possibility of having coverage reports into project-viewer-plus. Although I am strugling to have the coverage reporting :\ examples?

[cgalvarez]

@jccguimaraes and any other developer searching for adding code coverage reports to their Atom packages... I am working right now on developing an easy-to-mimic-workflow to get it. Right now it works for me, but I'm polishing it to be as easy as running npm run test:coverage. I'm considering mocking, stubbing, using several NPM packages that provides testing tools (like proxyquire)...

That's a lot of things to mix up. I hope pushing soon that polished workflow into atom-projects (my fork of atom-project-manager), so anyone who wants to learn my approach only needs to dig into my package.json scripts.

I'm just commenting this here to avoid polluting this PR thread, since it has served its purposed and it's merged. I prefer @Alhadis not to be bothered with notifications of something that isn't directly related to his project.

Although I am strugling to have the coverage reporting :\ examples?

Just watch for my repo. It's too long to explain, and some hackity hacks are needed to get it working. You will have a fully working example in the next one or two weeks (or even before!).

[jccguimaraes]

sure! amazing work! keep on

[Alhadis]

Sorry for taking so long to cut the release. 😞 v2.0.4 now published. 👍 Thanks again for your help! 😀

[cgalvarez]

Thanks to you, @Alhadis !! I was ready to ask you for that, because I've decided to release a NPM package for the coverage of Atom packages (so other users as @jccguimaraes can benefit of my approach), and I've set yours as a peer dependency which is currently pointing to the master branch of your repo.

Setting up a semver string is better, so thanks again!! 😉

[Alhadis]

De nada! 😉

And yes, semver is most certainly better than a hard-coded URL. That being said, v3.0.0 will break everything, so I'm trusting you've used ^ to specify the version.

[cgalvarez]

For sure! But thanks for the reminder anyway!

[jccguimaraes]

great work guys

[cgalvarez]

@Alhadis I've finally pushed my package to GitHub. It relies on this atom-mocha package to perform code coverage analysis on Atom packages. Again, thanks for your great package!

EDIT: @jccguimaraes take a look at that package, and let me know if you have any issues by opening an issue there!

[Alhadis]

I've given it your first star. 😀 Thanks for the heads up!

I'll admit your use of this has inspired me to make this more accessible to those running unit tests with Mocha, irrespective of whether those tests are even Atom/Electron-specific in nature. 😉 Hope to resume work on this some day. :D

[cgalvarez]

@Alhadis Thank you very much! Glad to have a positive influence on you 😊

Pin me whenever you resume your work 😉 . Hopefully I can offer you some insights to improve your package. Right now I'm integrating my own package with lots of cloud services to ensure quality/security (snyk, greenkeeper, bithound, codecov, coveralls, inch-ci) and configuring CI builds/checks (travis, appveyor).

Most of those cloud services are free for open source projects, and offer an invaluable information (at least to me). If you are interested in these things, let me know and I'll post them at #3