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

Add reporter options example #4225

Merged
merged 1 commit into from Mar 11, 2021
Merged

Add reporter options example #4225

merged 1 commit into from Mar 11, 2021

Conversation

pkuczynski
Copy link
Contributor

Description of the Change

I tried to add reporter options as reporterOptions or reporterOption and failed miserably for quite a while until I accidentally found out that it needs to be specified like in the example below.

Benefits

I think it's worth adding an example to the documentation to save other people's time.

@outsideris
Copy link
Member

We know you spent your time to figure this out, but this example configs aren't for explaining every option.
--reporter-option for XUNIT already described in our docs.

@outsideris outsideris added the status: wontfix typically a feature which won't be added, or a "bug" which is actually intended behavior label Apr 9, 2020
@outsideris outsideris closed this Apr 9, 2020
@pkuczynski
Copy link
Contributor Author

pkuczynski commented Apr 9, 2020

I know that, but what was surprising for me was the format. I was expecting to pass params like:

reporterOptions: {
     suiteName: "My Tests",
     output: "./build/tests.xml"
}

like its displayed across all the documentation, while instead, mocha expected this:

'reporter-options': [
     'suiteName="My Tests"',
     'output=./build/tests.xml'
   ],

This was very confusing for me and I guess it would be good to have it in those examples. Especially your other examples are much larger: https://github.com/mochajs/mocha/blob/master/example/config/.mocharc.yml

I can change the example to be more generic like in .mocharc.yml with foo, bar..

What you think?

@pkuczynski
Copy link
Contributor Author

@juergba @tj @boneskull what do you think?

@juergba
Copy link
Member

juergba commented Apr 10, 2020

IMO it is rather confusing, see eg. #4142.
In our configuration files reporter-option has to be an array. When passing it directly to the Mocha constructor, then it is an object.

@pkuczynski
Copy link
Contributor Author

My point exactly! There is no clear documentation about it, or at least I could not find it. However there is plenty documentation for passing an object to the constructor.

That’s why I strongly believe that adding example from this PR would help people...

Validation of mocharc structure would help too, but that’s a separate topic...

@outsideris
Copy link
Member

I understood what @pkuczynski pointed out. And I realized our .mocharc.yml is much verbose than others.
reporter-option is Array but each value is not same type with other Array type options. Therefore, IMO it is helpful for mocha newbies.

@outsideris outsideris reopened this Apr 11, 2020
Copy link
Member

@outsideris outsideris left a comment

Choose a reason for hiding this comment

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

IMO, if we add this in our configuration examples, we should add them into .mocharc.json and .mocharc.jsonc as well not only .mocharc.json.

@pkuczynski
Copy link
Contributor Author

I can do that tonight if you want?

@pkuczynski
Copy link
Contributor Author

I just updated my PR to align with .mocharc.yml. The only thing I noticed is that .mocharc.js had package property which .mocharc.yml does not. Not sure if this is a bug?

@outsideris outsideris added area: documentation anything involving docs or mochajs.org and removed status: wontfix typically a feature which won't be added, or a "bug" which is actually intended behavior labels Apr 16, 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.

@pkuczynski Thank you, I have some comments.

  • none of your commits did pass our CI tests
  • use the canonical option name reporter-option instead of its alias
  • there is no need to align with .mocharc.yml, please revert this change
  • .mocharc.js: your first commit is ok, just watch out for the option name and linting errors
  • .mocharc.json: align with .mocharc.js. Please remove the four comment lines
  • .mocharc.jsonc: align with .mocharc.js
    • remove // Camel-casing options are also accepted. The parsing of camel-cased option is buggy in certain edge cases.
    • change watchFiles to watch-files
    • change watchIgnore to watch-ignore

@pkuczynski
Copy link
Contributor Author

I have not modified anything else than example/config/.mocharc.js. Possibly I misunderstood @outsideris comment and added all options from yaml to json. I personally feel that having all options would make it easier for newbies to understand. Please confirm if I should revert and stick to the simple version was here before?

I suggest updating other files in a separate PR once we align on .mocharc.js

@coveralls
Copy link

coveralls commented Apr 17, 2020

Coverage Status

Coverage remained the same at 94.155% when pulling 491c6bc on pkuczynski:patch-1 into 21652d9 on mochajs:master.

@pkuczynski
Copy link
Contributor Author

Build is finally green now :)

@boneskull
Copy link
Member

thanks!!

Copy link
Member

@outsideris outsideris left a comment

Choose a reason for hiding this comment

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

I suggest updating other files in a separate PR once we align on .mocharc.js
Agreed.

IMO there isn't any reason we don't align with yaml for js.
Thank you.

@boneskull boneskull requested a review from juergba April 24, 2020 21:10
@boneskull
Copy link
Member

@juergba can you pls confirm your comments have been addressed; if so please merge

@boneskull boneskull added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label Apr 24, 2020
@boneskull
Copy link
Member

and by merge i mean "squash"

cambiph pushed a commit to milieuinfo/webcomponent-vl-ui-util that referenced this pull request Aug 11, 2020
cambiph pushed a commit to milieuinfo/webcomponent-vl-ui-accordion that referenced this pull request Aug 13, 2020
cambiph added a commit to milieuinfo/webcomponent-vl-ui-util that referenced this pull request Sep 29, 2020
… kunnen zien (#203)

* Initial commit

* Voeg npm run script toe

* Voeg path toe als dep en move build naar scripts folder

* Refactor

* Fix regex

* Refactor en toevoegen unit tests

* Refactor en unit testen

* Voeg wct-xunit dep toe

* Verplaats wct-xunit van devDependencies naar dependencies
Enable wct-xunit plugin in wct.local.conf

* Configure reporter via mocharc

* Geef reporter options door als array cfr mochajs/mocha#4225

* Fix pad naar result xml

* Config finetuning

* Installeer wct-mocha

* Verwijder wct-mocha (testen wilden niet meer runnen) en voeg wct-junit-converter als plugin toe

* Gebruik wct-xunit-reporter

* Fix paden

* Maak gitignore uniform en kopieer naar component

* Bugfix

* Bugfix

* Bugfix

* Bugfix

* Bugfix

* Bugfix

* Bugfix

* Kopieer gitignore en npmignore via templates omdat .gitignore en .npmginore niet in de gereleasde versie zitten

* copyfiles vervangen door cp

* Maak test result folder aan

* Verwijder intermediate folder results omdat de xunit plugin geen folders aanmaakt

* Verwijder intermediate folder results omdat de xunit plugin geen folders aanmaakt

* gitignore update

* Vervang rsync oplossing in docker-compose door gebruik te maken van dockerignore

* bugfix

* Expose test resultaten naar Bamboo

* Expose test resultaten naar Bamboo

* Merge with master

Co-authored-by: cambiph <philippe.cambien@gmail.com>
@Niceplace
Copy link

Hi folks ! Just checking in to say that this PR helped me understand how reporter-options parameter should be structured because it was not initially clear to me.

Any plans to merge this soon ? I think it could help a lot of people.

Cheers ! :)

I tried to add reporter options as `reporterOptions` or `reporterOption` and failed miserably for quite a while until I accidentally found out that it needs to be specified like in the example below. I think it's worth adding an example to the documentation to save other people's time.
This was referenced Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: documentation anything involving docs or mochajs.org 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