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

Added documentation on how to specify the reporter for browser mocha #4026

Merged
merged 1 commit into from Oct 14, 2019

Conversation

Lindsay-Needs-Sleep
Copy link
Contributor

@Lindsay-Needs-Sleep Lindsay-Needs-Sleep commented Sep 23, 2019

Description of the Change

Update documentation to include usage instructions on how to specify the reporter for browser tests.

This was absent, despite hinting that it could be done. It took quite awhile of digging through source to figure out how this could be correctly. This information should be available.

Alternate Designs

Thought about not including the list of possible strings, but decided I should because otherwise people will have to guess on that front as well.

Why should this be in core?

Because it's documentation.

Benefits

Save people time and frustration.

Possible Drawbacks

None.

Applicable issues

Issue #1592

@jsf-clabot
Copy link

jsf-clabot commented Sep 23, 2019

CLA assistant check
All committers have signed the CLA.

@juergba
Copy link
Member

juergba commented Sep 23, 2019

I guess the standard way to configure options, is using mocha.setup() as described some lines above:
mocha.setup ( {reporter: 'html'} )

I don't use Mocha in the browser, but I doubt the reporter lists is correct for browser usage.

@Lindsay-Needs-Sleep
Copy link
Contributor Author

Okay!
I'll try that out tomorrow and modify the documentation to match.

I'll try out a couple other reporters and see if they work. All the ones I listed are included in mocha.js. So it would seem that they technically should work (why include broken things). I'll file a bug if they don't.

I haven't dug around too much yet. Do you happen to know if mocha.js is exclusively for the browser? (I guess that is pretty relevant to which reporters are loaded by mocha.js!)

@Lindsay-Needs-Sleep
Copy link
Contributor Author

@juergba You are absolutely right! Thank you for the feedback!

I didn't realize this was how to do it correctly since the documentation on options for the browser is a bit sparse. So I added to the options documentation a bit instead.

I also tested all the available builtin reporters. They all "worked", (aka. none of them caused a failure). Most of the time the output was legible. I removed the list of reporters and put a link instead, (as they all basically correspond directly to the reporters list).

@Lindsay-Needs-Sleep
Copy link
Contributor Author

(Also not sure why the travis-ci build would fail when it is just documentation updates). I ran npm test, it all passed.

@coveralls
Copy link

coveralls commented Sep 25, 2019

Coverage Status

Coverage increased (+0.07%) to 92.779% when pulling 2ac64f9 on Lindsay-Needs-Sleep:master into f78e21f on mochajs:master.

docs/index.md Outdated Show resolved Hide resolved
@juergba
Copy link
Member

juergba commented Oct 1, 2019

I agree that the browser part of our docu is suboptimal. But again I have doubts about your changes.

The option list is not the actual one of the master branch, the Mocha constructor I guess.
Are you sure all these options do work in the browser?

What about the grep, probably also fgrep?

The browser may use the --grep as functionality. Append a query-string to your URL: ?grep=api.

What about color? invert is a CLI option?
Aren't the test files added manually to the browser, so file loading options do work only via CLI?

@Lindsay-Needs-Sleep
Copy link
Contributor Author

@juergba I just took the documentation from the code (lib/mocha.js I believe).

But, good piont. I did not test each option. So I guess it's technically better to have no documentation than have documentation that indicates non-existent/non-working functionality. I have removed the parts I did not personally test.

You were right about color. The documentation in the code was incorrect, it is not "color" but "useColors". I have updated that. And I did just test out useColors out of curiorsity:

useColors: false (reporter: spec)
image

useColors: true (reporter: spec)
image

@Lindsay-Needs-Sleep
Copy link
Contributor Author

Lindsay-Needs-Sleep commented Oct 8, 2019

I have removed the changes to the reporters' names documentation.
That can be addressed in Issue #4054.

This change only includes some minor improvements to the browser documentation.

@juergba juergba added area: browser browser-specific area: documentation anything involving docs or mochajs.org semver-major implementation requires increase of "major" version number; "breaking changes" labels Oct 10, 2019
@juergba
Copy link
Member

juergba commented Oct 10, 2019

Ok, let's make a last effort and finish this PR.

color / useColors:
the corresponding CLI option is color (with aliases c, colors), therefore the Mocha constructor should also accept an option color (the canonical name, no alias). There have been changes to the Mocha constructor a few weeks ago, in our master branch which is not published yet. Maybe you haven't tested with the master version of ./mocha.js. Anyway I didn't expect color to have an effect in the browser.

Add the options diff and inlineDiffs to your option list, they should also work in browser.

Please also add a comment that not all CLI options are supported in the browser, with a link to the CLI options. For the browser setup the camel-cased option names have to be used, eg. checkLeaks instead of check-leaks.

docs/index.md Outdated
```text
reporter {string|constructor} - Reporter** name or constructor.
ui {string} - Interface name.
useColors {boolean} - Use color TTY output from reporter?
Copy link
Member

Choose a reason for hiding this comment

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

it should be color

docs/index.md Outdated
Some available options:

```text
reporter {string|constructor} - Reporter** name or constructor.
Copy link
Member

Choose a reason for hiding this comment

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

please remove those **

lib/mocha.js Outdated
* @param {Object} [options.reporterOption] - Reporter settings object.
* @param {number} [options.retries] - Number of times to retry failed tests.
* @param {number} [options.slow] - Slow threshold value.
* @param {number|string} [options.timeout] - Timeout threshold value.
* @param {string} [options.ui] - Interface name.
* @param {boolean} [options.useColors] - Use color TTY output from reporter?
Copy link
Member

Choose a reason for hiding this comment

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

it should be color

@Lindsay-Needs-Sleep
Copy link
Contributor Author

@juergba Sure, I can go through those changes in a couple days. (I just have some very urgent work deadlines that I need to finish first. T.T)

@Lindsay-Needs-Sleep
Copy link
Contributor Author

I added diff and inlineDiffs. I tried them out as well. Nothing broke (after my changes). But I didn't see anything different. What are they supposed to do?

@Lindsay-Needs-Sleep
Copy link
Contributor Author

Lindsay-Needs-Sleep commented Oct 12, 2019

Also, changing the options to use color instead of useColors caused this[opt](opts[opt]); to break:

    mocha.setup = function(opts) {
      if (typeof opts === 'string') {
        opts = {ui: opts};
      }
      for (var opt in opts) {
        if (opts.hasOwnProperty(opt)) {
          this[opt](opts[opt]);
        }
      }
      return this;
    };

So I also changed:

Mocha.prototype.useColors = function(colors) {

to:

Mocha.prototype.color = function(colors) {

I had do similar changes for diff and inlineDiffs.

This caused these tests to fail:

1) Mocha
       constructor
         "before each" hook for "should not attempt to set timeout":
     TypeError: Cannot stub non-existent own property useInlineDiffs
      at Sandbox.stub (node_modules\sinon\lib\sinon\sandbox.js:308:19)
      at Context.<anonymous> (test\unit\mocha.spec.js:23:15)
      at callFn (lib\runnable.js:386:21)
      at Hook.Runnable.run (lib\runnable.js:373:7)
      at next (lib\runner.js:384:10)
      at Immediate.<anonymous> (lib\runner.js:421:5)

  2) Mocha
       #hideDiff()
         should set the diff option to false when param equals true:
     TypeError: mocha.hideDiff is not a function
      at Context.<anonymous> (test\unit\mocha.spec.js:230:13)
      at callFn (lib\runnable.js:386:21)
      at Test.Runnable.run (lib\runnable.js:373:7)
      at Runner.runTest (lib\runner.js:534:10)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:652:12
      at next (lib\runner.js:443:14)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:453:7
      at next (lib\runner.js:362:14)
      at Immediate.<anonymous> (lib\runner.js:421:5)

  3) Mocha
       #hideDiff()
         should set the diff option to true when param equals false:
     TypeError: mocha.hideDiff is not a function
      at Context.<anonymous> (test\unit\mocha.spec.js:236:13)
      at callFn (lib\runnable.js:386:21)
      at Test.Runnable.run (lib\runnable.js:373:7)
      at Runner.runTest (lib\runner.js:534:10)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:652:12
      at next (lib\runner.js:443:14)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:453:7
      at next (lib\runner.js:362:14)
      at Immediate.<anonymous> (lib\runner.js:421:5)

  4) Mocha
       #hideDiff()
         should set the diff option to true when the param is undefined:
     TypeError: mocha.hideDiff is not a function
      at Context.<anonymous> (test\unit\mocha.spec.js:242:13)
      at callFn (lib\runnable.js:386:21)
      at Test.Runnable.run (lib\runnable.js:373:7)
      at Runner.runTest (lib\runner.js:534:10)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:652:12
      at next (lib\runner.js:443:14)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:453:7
      at next (lib\runner.js:362:14)
      at Immediate.<anonymous> (lib\runner.js:421:5)

  5) Mocha
       #hideDiff()
         should be chainable:
     TypeError: mocha.hideDiff is not a function
      at Context.<anonymous> (test\unit\mocha.spec.js:248:20)
      at callFn (lib\runnable.js:386:21)
      at Test.Runnable.run (lib\runnable.js:373:7)
      at Runner.runTest (lib\runner.js:534:10)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:652:12
      at next (lib\runner.js:443:14)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:453:7
      at next (lib\runner.js:362:14)
      at Immediate.<anonymous> (lib\runner.js:421:5)

  6) Mocha
       #useColors()
         should set the color option to true:
     TypeError: mocha.useColors is not a function
      at Context.<anonymous> (test\unit\mocha.spec.js:358:13)
      at callFn (lib\runnable.js:386:21)
      at Test.Runnable.run (lib\runnable.js:373:7)
      at Runner.runTest (lib\runner.js:534:10)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:652:12
      at next (lib\runner.js:443:14)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:453:7
      at next (lib\runner.js:362:14)
      at Immediate.<anonymous> (lib\runner.js:421:5)

  7) Mocha
       #useColors()
         should not create the color property:
     TypeError: mocha.useColors is not a function
      at Context.<anonymous> (test\unit\mocha.spec.js:364:13)
      at callFn (lib\runnable.js:386:21)
      at Test.Runnable.run (lib\runnable.js:373:7)
      at Runner.runTest (lib\runner.js:534:10)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:652:12
      at next (lib\runner.js:443:14)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:453:7
      at next (lib\runner.js:362:14)
      at Immediate.<anonymous> (lib\runner.js:421:5)

  8) Mocha
       #useColors()
         should be chainable:
     TypeError: mocha.useColors is not a function
      at Context.<anonymous> (test\unit\mocha.spec.js:370:20)
      at callFn (lib\runnable.js:386:21)
      at Test.Runnable.run (lib\runnable.js:373:7)
      at Runner.runTest (lib\runner.js:534:10)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:652:12
      at next (lib\runner.js:443:14)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:453:7
      at next (lib\runner.js:362:14)
      at Immediate.<anonymous> (lib\runner.js:421:5)

  9) Mocha
       #useInlineDiffs()
         should set the inlineDiffs option to true when param equals true:
     TypeError: mocha.useInlineDiffs is not a function
      at Context.<anonymous> (test\unit\mocha.spec.js:377:13)
      at callFn (lib\runnable.js:386:21)
      at Test.Runnable.run (lib\runnable.js:373:7)
      at Runner.runTest (lib\runner.js:534:10)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:652:12
      at next (lib\runner.js:443:14)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:453:7
      at next (lib\runner.js:362:14)
      at Immediate.<anonymous> (lib\runner.js:421:5)

  10) Mocha
       #useInlineDiffs()
         should set the inlineDiffs option to false when param equals false:
     TypeError: mocha.useInlineDiffs is not a function
      at Context.<anonymous> (test\unit\mocha.spec.js:383:13)
      at callFn (lib\runnable.js:386:21)
      at Test.Runnable.run (lib\runnable.js:373:7)
      at Runner.runTest (lib\runner.js:534:10)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:652:12
      at next (lib\runner.js:443:14)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:453:7
      at next (lib\runner.js:362:14)
      at Immediate.<anonymous> (lib\runner.js:421:5)

  11) Mocha
       #useInlineDiffs()
         should set the inlineDiffs option to false when the param is undefined:
     TypeError: mocha.useInlineDiffs is not a function
      at Context.<anonymous> (test\unit\mocha.spec.js:389:13)
      at callFn (lib\runnable.js:386:21)
      at Test.Runnable.run (lib\runnable.js:373:7)
      at Runner.runTest (lib\runner.js:534:10)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:652:12
      at next (lib\runner.js:443:14)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:453:7
      at next (lib\runner.js:362:14)
      at Immediate.<anonymous> (lib\runner.js:421:5)

  12) Mocha
       #useInlineDiffs()
         should be chainable:
     TypeError: mocha.useInlineDiffs is not a function
      at Context.<anonymous> (test\unit\mocha.spec.js:395:20)
      at callFn (lib\runnable.js:386:21)
      at Test.Runnable.run (lib\runnable.js:373:7)
      at Runner.runTest (lib\runner.js:534:10)
      at S:\Projects\MiloProductions\00 Old and reference Repos\mocha\lib\runner.js:652:12
      at next (lib\runner.js:443:14)

So should I leave the prototype functions alone, and then be modifying the Mocha.setup function to not use this[opt](opts[opt]); and instead manually call the corresponding function for each option?

This seems way beyond the scope of my pull request intent. I just wanted to let people know how to use custom reporters in the browser. T.T

This problem also probably extends to many of the options. Could we create a new issue specifically for dealing with all the browser options? (Finding out which ones actually do things, have proper names, etc)

Lindsay-Needs-Sleep added a commit to Lindsay-Needs-Sleep/mocha that referenced this pull request Oct 12, 2019
…in browser.

You can pass the constructor function of your custom reporter in options and mocha will use it.
@juergba
Copy link
Member

juergba commented Oct 12, 2019

Yes, I'm sorry. Lets squash this PR down to the absolute minimum, the reporter setup thing, and then finish it.
I will have a look at the error messages.

@juergba
Copy link
Member

juergba commented Oct 12, 2019

I just have tested most of the builtin reporters, the only one which works in the browser on my laptop (Chrome/Windows10) is "HTML". All the others don't output anything.
You commented earlier that all builtin reporters were producing an output. Can you explain?
When I misspell the reporter (eg. "SPEC"), then the default reporter ("html") is used.
Otherwise with correct spelling "spec" there is no output at all.

@Lindsay-Needs-Sleep
Copy link
Contributor Author

Okay, I slimmed it down.
If you would prefer a different format or wording, please fee free to edit it, or tell me exactly what would be preferred. I really want to do no more than 1 more revision to this! >.<

@juergba
Copy link
Member

juergba commented Oct 13, 2019

Since I have one try left ...

  • If you check the "deploy/netlify preview" (under "checks") the format is wrong, one column at the left.
  • I'm not happy with the console output. There are some illegible signs, and I don't want to be flooded by new issues about that. But obviously there are reasons to do so.
    To finish this: we don't recommend to use CLI buitlin reporters for browser, but it's possible. So please mention this with cautious words (you can put your cat in the microwave, but don't blame us ...).

@juergba juergba removed the semver-major implementation requires increase of "major" version number; "breaking changes" label Oct 13, 2019
@Lindsay-Needs-Sleep
Copy link
Contributor Author

I updated it again. How does it look now? (I'll change it again if need be.)

If you check the "deploy/netlify preview" (under "checks") the format is wrong, one column at the left.

This is very cool. Thank you for pointing this out. (And the mistake.) Apparently my markdown editor preview is not very accurate. (I'll check back when the deploy/netlify is done)

@juergba
Copy link
Member

juergba commented Oct 13, 2019

The cat was meant as a picture, I was hoping you as english native would translate this into some smooth words ... meaning: you can use those CLI reporters in the browser, but they are not made/supported for that purpose.

I will finish this PR tomorrow with maybe:

It is possible to use built-in reporters as well, but their employment in browsers is neither recommended nor supported.

Thanks!

You can pass the constructor function of your custom reporter in options and mocha will use it.
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.

@Lindsay-Needs-Sleep thank you!
I also run npm start docs which updated some automatically created docu parts.

@juergba juergba merged commit 3a0ef50 into mochajs:master Oct 14, 2019
@juergba juergba added this to the next milestone Oct 14, 2019
@Lindsay-Needs-Sleep
Copy link
Contributor Author

The cat was meant as a picture, I was hoping you as english native would translate this into some smooth words ... meaning: you can use those CLI reporters in the browser, but they are not made/supported for that purpose.

Haha, sorry! I liked it, it made me laugh, and it definitely got the point across. (But, I guess it was a bit extreme. :p) (I wouldn’t have guessed that english is not your first language!)

Thank you for finishing this pull request though! :)

@boneskull boneskull added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label Oct 14, 2019
@juergba juergba modified the milestones: 6.2.1, 6.2.2 Oct 18, 2019
boneskull pushed a commit that referenced this pull request Oct 18, 2019
You can pass the constructor function of your custom reporter in options and mocha will use it.
# Conflicts:
#	docs/index.md
@boneskull boneskull added the landed-on-v6.2.x cherry-picked on v6.2.x branch from master label Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: browser browser-specific area: documentation anything involving docs or mochajs.org landed-on-v6.2.x cherry-picked on v6.2.x branch from master 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

6 participants