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

#2819 - this.skip() in before fails to skip tests with nested describe calls #2836

Closed
wants to merge 1 commit into from

Conversation

Elergy
Copy link
Contributor

@Elergy Elergy commented May 27, 2017

this.skip() in before fails to skip tests with nested describe calls

this.skip() in before fails to skip tests with nested describe calls
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 5267c53 on Elergy:issue/2819 into ** on mochajs:master**.

@Elergy Elergy changed the title #2819 #2819 - this.skip() in before fails to skip tests with nested describe calls May 27, 2017
@boneskull
Copy link
Contributor

@Elergy See conversation in #2571. I'm not sure if we ever came to an agreement about how this should work. There are differing opinions.

@boneskull boneskull added the status: needs review a maintainer should (re-)review this pull request label Jun 3, 2017
@Elergy
Copy link
Contributor Author

Elergy commented Jun 3, 2017

Thank you, I didn't see #2571 before.
I'm going to understand the scenarios and return :-)

Copy link

@fearphage fearphage left a comment

Choose a reason for hiding this comment

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

I have a question about one of the changes and I think the failure messages could be improved.

});

it('should skip this test', function () {
expect(2).to.equal(1);

Choose a reason for hiding this comment

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

In all the failure cases, you should call fail directly: expect().to.fail('message here')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

lib/runnable.js Outdated
* Mark the runnable as pending or not pending
* @param pending
*/
Runnable.prototype.setPending = function (pending) {

Choose a reason for hiding this comment

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

What is the value of switching from property to a method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In most cases, it's better to use setters instead of using properties directly.

It's a common problem with using properties (and I encountered this problem while fixing this bug) that it's not easy to understand where someone changes the property.

You know that the property was changed, but you don't know when - all you can do is to debug the whole application.
You can search for a string 'pending = ', but it can be a problem when several objects contain fields with the same name.

if you have a setter, you can easily set a breakpoint inside the setter and find who called this method very quickly.

@boneskull boneskull force-pushed the master branch 2 times, most recently from 4547268 to 7613521 Compare May 4, 2018 16:47
@craigtaub
Copy link
Contributor

@Elergy i think the fix for this has now gone into master (via #3225). Closing this PR.

@craigtaub craigtaub closed this Dec 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs review a maintainer should (re-)review this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants