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

options.reporterOptions are used for progress reporter #2649

Merged
merged 1 commit into from
Dec 14, 2017

Conversation

canoztokmak
Copy link
Contributor

Progress reporter had multiple options for configuring the reporter,
but it needed the optional fields under options object, while
those fields are provided under options.reporterOptions.
This commit enables usage of optional fields from reporterOptions.

@jsf-clabot
Copy link

jsf-clabot commented Dec 28, 2016

CLA assistant check
All committers have signed the CLA.

@canoztokmak canoztokmak force-pushed the bugfix/progress-options branch from 2119db7 to ba7049d Compare December 28, 2016 20:57
Progress reporter had multiple options for configuring the reporter,
but it needed the optional fields under `options` object, while
those fields are provided under `options.reporterOptions`.
This commit enables usage of optional fields from `reporterOptions`.
@canoztokmak canoztokmak force-pushed the bugfix/progress-options branch from ba7049d to e95f05f Compare December 30, 2016 08:36
@canoztokmak canoztokmak changed the title options.reporterOptions are used for progress reporter options.reporterOptions are used for progress reporter Feb 17, 2017
@canoztokmak
Copy link
Contributor Author

solution for issue #2661

@drazisil drazisil added type: bug a defect, confirmed by a maintainer status: needs review a maintainer should (re-)review this pull request area: reporters involving a specific reporter pr-needs-tests and removed status: needs review a maintainer should (re-)review this pull request pr-needs-tests labels Mar 30, 2017
@canoztokmak
Copy link
Contributor Author

ping

@xxczaki
Copy link
Contributor

xxczaki commented Dec 13, 2017

@boneskull i think this should be merged & the issue #2661 closed + this is a pretty old pr :/

@boneskull
Copy link
Contributor

old PR's happen

@boneskull
Copy link
Contributor

LGTM, thanks

@boneskull boneskull added semver-patch implementation requires increase of "patch" version number; "bug fixes" and removed status: needs review a maintainer should (re-)review this pull request labels Dec 14, 2017
@boneskull boneskull merged commit 78b686f into mochajs:master Dec 14, 2017
@boneskull
Copy link
Contributor

@canoztokmak Can you please try this again? It seems this introduced breaking changes

@canoztokmak
Copy link
Contributor Author

Hey @boneskull,

The reporter options are passed as a field inside options. See example below:
https://github.com/mochajs/mocha/blob/master/lib/reporters/xunit.js#L52

But in test, the options are passed directly. So the tests are validating the current behaviour, which is wrong. See below:
https://github.com/mochajs/mocha/blob/master/test/reporters/progress.spec.js#L99

There was no tests when I introduced the fix, that's why build did not failed back then. I think I can fix the tests and send another PR including with this change in a few days.

@canoztokmak
Copy link
Contributor Author

@boneskull I've just sent another PR to solve the issue.
#3182

@boneskull boneskull added this to the reversed milestone Dec 29, 2017
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 semver-patch implementation requires increase of "patch" version number; "bug fixes" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants