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.

Sign up for GitHub

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

Jump to bottom

Sets the allowUncaught parameter for hooks. #2303

Closed
wants to merge 3 commits into from
Closed

Sets the allowUncaught parameter for hooks. #2303

wants to merge 3 commits into from

Conversation

dima117
Copy link

@dima117 dima117 commented Jun 10, 2016 •
edited

@dima117 dima117 changed the title https://github.com/mochajs/mocha/issues/2302 Sets the allowUncaught parameter for hooks. Jun 10, 2016
@boneskull
Copy link
Member

@dima117 Thanks! Can you please add a test in test/runner.js?

dima117 added a commit to dima117/mocha that referenced this pull request Jun 14, 2016
@dima117
test for mochajs#2303
46654cd
@dima117
Copy link
Author

dima117 commented Jun 14, 2016

I added a test for allowUncaught parameter in hooks.

@dima117
Copy link
Author

dima117 commented Jun 17, 2016

Do I need to do something else for this PR?

@dima117
Copy link
Author

dima117 commented Jun 24, 2016

@boneskull What is the state of this PR?

@ScottFreeCode
Copy link
Contributor

Is it safe to call suite.beforeEach inside a test?

@dima117
Copy link
Author

dima117 commented Jul 6, 2016

I think yes. Why it may be unsafe?

@boneskull
Copy link
Member

@dima117 Apologies for not getting to this earlier.

boneskull
boneskull reviewed Jul 7, 2016
View reviewed changes
test/runner.js
@@ -315,6 +315,17 @@ describe('Runner', function(){
});

describe('allowUncaught', function() {
var immediately;
Copy link
Member

Choose a reason for hiding this comment

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

Why are we monkeypatching Runner.immediately()?

Copy link
Author

Choose a reason for hiding this comment

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

It was done in order to run the hook synchronously.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to do that?

Copy link
Author

Choose a reason for hiding this comment

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

We need it to catch the exception in the test method, instead of catching it on a global error handler.

@boneskull boneskull added the status: waiting for author waiting on response from OP - more information needed label Aug 28, 2016
@dima117
Copy link
Author

dima117 commented Sep 20, 2016

Hello! What is the status of this PR?

@boneskull boneskull added status: needs review a maintainer should (re-)review this pull request and removed status: waiting for author waiting on response from OP - more information needed pr-needs-work labels Sep 21, 2016
@boneskull boneskull closed this Jan 17, 2018
@boneskull
Copy link
Member

I may use some of this, though I don't expect the original author to sign a CLA at this point

@boneskull
Copy link
Member

see #2302

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
status: needs review a maintainer should (re-)review this pull request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dima117 @boneskull @ScottFreeCode