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.

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 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

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
Member

old PR's happen

@boneskull
Copy link
Member

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
Member

@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.

None yet

5 participants