-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
I guess the standard way to configure options, is using I don't use Mocha in the browser, but I doubt the reporter lists is correct for browser usage. |
Okay! 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!) |
67686eb
to
5bfbdb4
Compare
@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). |
(Also not sure why the travis-ci build would fail when it is just documentation updates). I ran |
5bfbdb4
to
1b7d714
Compare
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 What about the
What about |
8a01b3d
to
ff6b64f
Compare
@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: |
ff6b64f
to
455505e
Compare
I have removed the changes to the reporters' names documentation. This change only includes some minor improvements to the browser documentation. |
Ok, let's make a last effort and finish this PR.
Add the options 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. |
docs/index.md
Outdated
```text | ||
reporter {string|constructor} - Reporter** name or constructor. | ||
ui {string} - Interface name. | ||
useColors {boolean} - Use color TTY output from reporter? |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be color
@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) |
I added |
Also, changing the options to use
So I also changed:
to:
I had do similar changes for This caused these tests to fail:
So should I leave the prototype functions alone, and then be modifying the Mocha.setup function to not use 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) |
…in browser. You can pass the constructor function of your custom reporter in options and mocha will use it.
Yes, I'm sorry. Lets squash this PR down to the absolute minimum, the reporter setup thing, and then finish it. |
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. |
a5c5b24
to
40bbaaa
Compare
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:
Thanks! |
You can pass the constructor function of your custom reporter in options and mocha will use it.
There was a problem hiding this 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.
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! :) |
You can pass the constructor function of your custom reporter in options and mocha will use it. # Conflicts: # docs/index.md
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