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

Fix backwards compatibility break for reporterOptions #4153

Merged
merged 1 commit into from Jan 20, 2020

Conversation

holm
Copy link

@holm holm commented Jan 14, 2020

Description of the Change

In #4004 there was a change to use the documented reporterOption in favour of the setting that had always been used in practice called reporterOptions. This broke a lot of configurations that used the reporterOptions, which was the only way it every worked AFAIK. This is also clear in that all the built-in reporters use this name.

This changes the documentation to specify reporterOptions instead and ensure that any one that has switched to reporterOption after upgrade, still works.

Alternate Designs

reporterOption could have been chosen as the new preferred way, but objects with options are generally referred to in their plural form in JS. I think the root cause of the confusion comes from the command line option reporter-option. I think it makes sense that this is singular, since it is a single option, but it is compiled into a reporterOptions object.

Why should this be in core?

To fix related issues like michaelleeallen/mocha-junit-reporter#106 (comment)

Benefits

Make it easier to upgrade to Mocha 7

Possible Drawbacks

Slight overhead of supporting two aliases

Applicable issues

@jsf-clabot
Copy link

jsf-clabot commented Jan 14, 2020

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Jan 14, 2020

Coverage Status

Coverage increased (+0.07%) to 92.925% when pulling 17adf4a on peakon:issue/4142 into a24683f on mochajs:master.

Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

I'm against using reporterOptions in the Mocha constructor.
We are using options in differnt context - CLI, Mocha constructor, browser environment - and there must be consistent rules for the option naming. Otherwise it's just a mess, for the user and for maintainance.

  • CLI: use whatever canonical, alias name or its camel-cased version
  • constructor: canonical or its camel-cased version
  • browser: canonical or its camel-cased version

Version 7 is a major release and the best moment to implement this change.

Edit: some of the problems you report, must have a different cause, since they use Mocha v6.2.0

@holm
Copy link
Author

holm commented Jan 14, 2020

I'm unsure what change you are requesting here. I'm not sure why the change I am proposing is inconsistent with anything.

Surely you do not want to require everyone to update their settings and all reporters to release new versions of their packages, which then in turn have to manage backwards compatibility with older versions of Mocha?

@juergba
Copy link
Member

juergba commented Jan 14, 2020

reporterOptions is not a canonical name, but an alias.

I'm planning to reduce the options passed into Mocha constructor (in our case: reporter-option, reporter-options, reporterOption, reporterOptions, O). So I don't agree to the change from reporterOption to reporterOptions as expected option name.

For backwards compability (L105) we could add reporterOptions as fallback, but reporterOption should remain the option to be.

@holm
Copy link
Author

holm commented Jan 14, 2020

The Mocha constructor is never called with reporter-option or reporter-options AFAIK. That is only done via the CLI.

Searching the code base, up until your change, reporterOption was never used outside the documentation mistake. If anyone had passed it in, it would simply be ignored. From what I can tell you want to change it to use reporter-option on the command line and reporterOption in the rest of the code. I don't really see why that is better than my proposal of reporter-option in the CLI and reporterOptions in the rest of the code. They are not the same, one is a single option, the other is a set of options.

Since reporterOptions have always been used by everyone, rather than try to force this change on everyone, wouldn't it be better to just use my suggestion?

@juergba
Copy link
Member

juergba commented Jan 14, 2020

No, I don't want to change anything for use in CLI, everything remains as is.
In CLI you can use comma separators, so --reporter-option is not only a single option.

For the constructor, I need this rule for all options: "canonical or its camel-cased version".
"[...] have always been used [...]" is not a good argument, especially in a major release. Furthermore if we keep backwards compability with the mentioned fallback.

@juergba juergba added this to the next milestone Jan 19, 2020
Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

I added some detailed comments to avoid further misunderstandings. Please also adapt the test accordingly.

lib/mocha.js Outdated Show resolved Hide resolved
lib/mocha.js Outdated Show resolved Hide resolved
lib/mocha.js Outdated Show resolved Hide resolved
@holm holm force-pushed the issue/4142 branch 2 times, most recently from 0e30ed8 to dd5ef42 Compare January 19, 2020 16:10
@holm holm requested a review from juergba January 19, 2020 18:21
Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@holm thank you, just two small comments.

test/unit/mocha.spec.js Outdated Show resolved Hide resolved
test/unit/mocha.spec.js Show resolved Hide resolved
@juergba juergba added area: reporters involving a specific reporter semver-patch implementation requires increase of "patch" version number; "bug fixes" area: usability concerning user experience or interface labels Jan 20, 2020
In mochajs#4004 there was a change to use the documented `reporterOption` in favour of the setting that had always been used in practice called `reporterOptions`. This broke a lot of configurations that used the `reporterOptions`, which was the only way it every worked AFAIK.

This changes the documentation to specify `reporterOptions` instead and ensure that any one that has switched to `reporterOption` after upgrade, still works.
Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@holm thank you

@juergba juergba merged commit 9c10ada into mochajs:master Jan 20, 2020
@holm holm deleted the issue/4142 branch January 24, 2020 22:21
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>
kellyselden pushed a commit to CrowdStrike/faltest that referenced this pull request Jan 27, 2020
kellyselden pushed a commit to CrowdStrike/faltest that referenced this pull request Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: reporters involving a specific reporter 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

4 participants