-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix weird "pending" behavior; closes #2286 #2623
Conversation
@ScottFreeCode any further comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a maintainer, but I've added my thoughts that would make the code more explicit and easier to maintain.
|
||
function spy (skip) { | ||
function wrapped () { | ||
if ((!wrapped.runCount++ || mode === 'always') && skip) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrapped.runCount++ === 0
would add a bit more clarity and make it very explicit what you're looking for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your thoughts!
I usually just prefer to use !
when a falsy check is adequate. It's a common style, so I'm not too concerned about it.
Though, some prefer explicit addition (+
) over increment operator(s). This is more obvious for readers of JavaScript who are, say, unfamiliar with the difference between foo++
and ++foo
(or even +foo
).
But, if it's opaque, I'll find another way to skin this particular cat.
} | ||
|
||
testPendingRunnables('beforeAll', [ | ||
true, // beforeAll |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Booleans that map to calls are not intuitive. What if you used strings instead?
Possibly use beforeAll
and beforeAll-called-skip
(or something similar) to denote the state?
Then you wouldn't need comments to explain what everything means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good suggestion.
This entire test is a pretty rotten hack, which I am unhappy with. If anyone can think of a better solution--and/or implement the requisite harness(es)--I'd be grateful!
I took a stab at it. I can't push to this branch though. |
@boneskull struggling to confirm if this is still useful. Could you confirm pls? |
continuation of #2571 with behavior suggested by @laughinghan.
this is targeted to
beta
branch for pre-release