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

Throw a descriptive error when a non-function is given to a runnable #4133

Merged
merged 1 commit into from Jan 19, 2020

Conversation

Zirak
Copy link
Contributor

@Zirak Zirak commented Dec 20, 2019

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

Given a test such as:

it('foobars', 4);

mocha used to throw the following error: fn.call is not a function.

After applying this patch, it will throw a more descriptive error message:
A runnable must be passed a function as its second argument.

(Thanks to ssube for helping with the error wording.)

Alternate Designs

Why should this be in core?

This deals with an error builtin to mocha.

Benefits

While the previous error is technically true, I have personally spent some minutes of my life chasing what looked like a bug in the test itself, and not in the call to it. A more descriptive error message helps bring attention to where the problem originates.

The error message itself is welcome to change. One other possible wording is replacing runnable, a phrase internal to mocha, with test function, which may be more intuitive to users.

Possible Drawbacks

  • Any code relying on runner.run returning the original error will now fail.
  • This error is reported as if the test itself failed, which may leave the user confused still, wondering what's wrong with their test and why is it telling them such things, and still not end up looking at the function signature.
  • The test for this function is, in my opinion, lacking. It merely tests how mocha behaves when it is passed a number, and not one of many possible values. Is there any prior art in mocha for such "generative" tests, for instance where the it string is constructed from variables? A common approach, which may be controversial to introduce if not already present, is something akin to:
var notFunctions = [4, 'four', false, ...];
notFunctions.forEach(function(val) {
    it('handles the value ' + inspect(val), testfn);
});

Thank you for your time.

@coveralls
Copy link

coveralls commented Dec 20, 2019

Coverage Status

Coverage increased (+0.08%) to 92.889% when pulling 7f649bd on Zirak:handle-non-function-runnable into 7d78f20 on mochajs:master.

var runnable = new Runnable('foo', 4);

runnable.run(function(err) {
expect(err.message, 'not to match', /fn\.call/);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think i'd rather it tested for the new message rather than lack of old one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you rather the match be the error message verbatim, or only a portion of?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to match the error message verbatim.

@craigtaub
Copy link
Contributor

craigtaub commented Jan 6, 2020

Thanks for the input.

Thinking about your drawbacks points, overall I can't think of anything which would block this fix.

Any code relying on runner.run returning the original error will now fail.

That sounds ok, i'd hope if it wasn't we'd have a test for it.

This error is reported as if the test itself failed,....

I mean isn't the point of this PR to give more context to the user to check the function signature? Not perfect but better than whats there.

Given a test such as:

```js
it('foobars', 4);
```

mocha used to throw the following error: `fn.call is not a function`.

While technically true, I have personally spent some minutes of my life chasing
what looked like a bug in the test itself, and not in the call to `it`. A more
descriptive error message helps bring attention to where the problem originates.

(Thanks to [ssube](https://github.com/ssube) for helping with the error wording.)
@Zirak Zirak force-pushed the handle-non-function-runnable branch from 185561c to 7f649bd Compare January 10, 2020 14:34
@craigtaub craigtaub added semver-patch implementation requires increase of "patch" version number; "bug fixes" area: usability concerning user experience or interface labels Jan 19, 2020
@craigtaub craigtaub merged commit a24683f into mochajs:master Jan 19, 2020
@craigtaub craigtaub added this to the next milestone Jan 19, 2020
@Zirak Zirak deleted the handle-non-function-runnable branch January 19, 2020 19:22
@Zirak
Copy link
Contributor Author

Zirak commented Jan 19, 2020

Thanks for the merge!
There is another PR of mine, #4032, that if you have some time could use a maintainer's quick look-over.

bors bot added a commit to IMA-WorldHealth/bhima that referenced this pull request Jan 26, 2020
4129: Update mocha to the latest version 🚀 r=jniles a=greenkeeper[bot]


## The devDependency [mocha](https://github.com/mochajs/mocha) was updated from `6.2.2` to `7.0.1`.
This version is **not covered** by your **current version range**.

If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update.

---

**Publisher:** [juergba](https://www.npmjs.com/~juergba)
**License:** MIT

<details>
<summary>Release Notes for v7.0.1</summary>

<h1>7.0.1 / 2020-01-25</h1>
<h2><g-emoji class="g-emoji" alias="bug" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f41b.png">🐛</g-emoji> Fixes</h2>
<ul>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/issues/4165" data-hovercard-type="pull_request" data-hovercard-url="/mochajs/mocha/pull/4165/hovercard">#4165</a>: Fix exception when skipping tests programmatically (<a href="https://urls.greenkeeper.io/juergba"><strong>@juergba</strong></a>)</li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/issues/4153" data-hovercard-type="pull_request" data-hovercard-url="/mochajs/mocha/pull/4153/hovercard">#4153</a>: Restore backwards compatibility for <code>reporterOptions</code> (<a href="https://urls.greenkeeper.io/holm"><strong>@holm</strong></a>)</li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/issues/4150" data-hovercard-type="pull_request" data-hovercard-url="/mochajs/mocha/pull/4150/hovercard">#4150</a>: Fix recovery of an open test upon uncaught exception (<a href="https://urls.greenkeeper.io/juergba"><strong>@juergba</strong></a>)</li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/issues/4147" data-hovercard-type="pull_request" data-hovercard-url="/mochajs/mocha/pull/4147/hovercard">#4147</a>: Fix regression of leaking uncaught exception handler (<a href="https://urls.greenkeeper.io/juergba"><strong>@juergba</strong></a>)</li>
</ul>
<h2><g-emoji class="g-emoji" alias="book" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f4d6.png">📖</g-emoji> Documentation</h2>
<ul>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/issues/4146" data-hovercard-type="pull_request" data-hovercard-url="/mochajs/mocha/pull/4146/hovercard">#4146</a>: Update copyright &amp; trademark notices per OJSF (<a href="https://urls.greenkeeper.io/boneskull"><strong>@boneskull</strong></a>)</li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/issues/4140" data-hovercard-type="pull_request" data-hovercard-url="/mochajs/mocha/pull/4140/hovercard">#4140</a>: Fix broken links (<a href="https://urls.greenkeeper.io/KyoungWan"><strong>@KyoungWan</strong></a>)</li>
</ul>
<h2><g-emoji class="g-emoji" alias="nut_and_bolt" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f529.png">🔩</g-emoji> Other</h2>
<ul>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/issues/4133" data-hovercard-type="pull_request" data-hovercard-url="/mochajs/mocha/pull/4133/hovercard">#4133</a>: Print more descriptive error message (<a href="https://urls.greenkeeper.io/Zirak"><strong>@Zirak</strong></a>)</li>
</ul>
</details>

<details>
<summary>Commits</summary>
<p>The new version differs by 61 commits ahead by 61, behind by 17.</p>
<ul>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/d0f04e994f3e78939f0a947ef064881c7fec5188"><code>d0f04e9</code></a> <code>Release v7.0.1</code></li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/2277958e32f48bed10f0cb2ceaf01e7b8045af35"><code>2277958</code></a> <code>update CHANGELOG for v7.0.1 [ci skip]</code></li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/0be3f78491bbbcdc4dcea660ee7bfd557a225d9c"><code>0be3f78</code></a> <code>Fix exception when skipping tests programmatically  (#4165)</code></li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/c0f1d1456dbc068f0552a5ceaed0d9b95e940ce1"><code>c0f1d14</code></a> <code>uncaughtException: fix recovery when current test is still running (#4150)</code></li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/9c10adab3340abd8baff147cb595256234d88de6"><code>9c10ada</code></a> <code>Fix backwards compability break for reporterOptions</code></li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/a24683fd9273d0896a177d70c2368ada4f2c4882"><code>a24683f</code></a> <code>Throw a descriptive error when a non-function is given to a runnable (#4133)</code></li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/579fd09db39a55b44c1f553df05c918bc62867be"><code>579fd09</code></a> <code>update copyright &amp; trademark notices per OJSF; closes #4145</code></li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/0e1ccbb915ba8c2f73134af5bebd357f3329b9b7"><code>0e1ccbb</code></a> <code>Fix leaking global 'uncaughtException' handler (#4147)</code></li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/7d78f209c6a4f8ef4eba584fe10515fd3901830e"><code>7d78f20</code></a> <code>Broken links in docs (#4140)</code></li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/69339a3e7710a790b106b922ce53fcb87772f689"><code>69339a3</code></a> <code>Release v7.0.0</code></li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/99e085f1fb924deeb87290adb476f4e375e72392"><code>99e085f</code></a> <code>update CHANGELOG for v7.0.0 [ci skip]</code></li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/35cf39b14eae6dbd1fb364c215093095d5912ebc"><code>35cf39b</code></a> <code>Add reporter alias names to docs (#4127)</code></li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/3bd2d28bfc99b5f71efc9ef332ae9ac4a5d90de8"><code>3bd2d28</code></a> <code>Forbid this.skip() within afterAll hooks (#4136)</code></li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/24c22bef53e4539dd17b0d3b2123953bb8a3a883"><code>24c22be</code></a> <code>Fix hook pattern of this.skip() in beforeEach hooks (#3741)</code></li>
<li><a href="https://urls.greenkeeper.io/mochajs/mocha/commit/1412dc80d87d0479f7f1d60202da2b33c90eb939"><code>1412dc8</code></a> <code>XUnit reporter should handle exceptions during diff generation (#4068)</code></li>
</ul>
<p>There are 61 commits in total.</p>
<p>See the <a href="https://urls.greenkeeper.io/mochajs/mocha/compare/843a322f9e7724e4a75ceff1920caf24da94bdf2...d0f04e994f3e78939f0a947ef064881c7fec5188">full diff</a></p>
</details>

---

<details>
  <summary>FAQ and help</summary>

  There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html). If those don’t help, you can always [ask the humans behind Greenkeeper](https://github.com/greenkeeperio/greenkeeper/issues/new).
</details>

---


Your [Greenkeeper](https://greenkeeper.io) bot 🌴



Co-authored-by: greenkeeper[bot] <23040076+greenkeeper[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: usability concerning user experience or interface semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants