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

Indicate which test drooling assert.async callbacks came from #1599

Merged
merged 5 commits into from
Apr 24, 2021

Conversation

smcclure15
Copy link
Member

This is a more tightly scoped version of #1595, tackling only the assert.async callback error handling. The remainder (handling the onError and global failure scenarios) grew larger than expected, so I'd like to slice this part off for iterative improvements, and circle back on the more generic issues.

The source change is straightforward.
Whenever a "done" callback is fired outside of the originating test, we currently see this:

assert.async callback called after test finished.

We can tease that apart for 2 distinct use cases.
If we're in another test, we can use pushFailure to show:

`assert.async` callback from test 'foobar' was called during this test.

Or if we're outside of the tests, we can throw:

`assert.async` callback from test 'foobar' called after tests finished.

I added a handful of tests - some to exercise these new paths, and some for related workflows by simply validating current behavior to avoid any unexpected changes, even if the current behavior is less than ideal.

src/assert.js Outdated
@@ -86,8 +86,17 @@ class Assert {
const resume = internalStop( test );

return function done() {

if ( config.current === undefined ) {
throw new Error( "`assert.async` callback from test '" +
Copy link
Member

Choose a reason for hiding this comment

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

Very slight preference for an escaped double quote here to match the formatting of errors from assert.rejects.

I might explore doing a larger consistency clean up over these kinds of messages. One of the things I quite liked from another patch recently is placing the tes name on its own line separate from the error. That seems to help both in brevity (problem first, then context), as well as less likely to become difficult to read when test names contain quotes and other symbols, e.g. assert.async() callback after test finished.\nTest: foo bar's "baz". But, that's for another one.

I'll swap the quote here when landing the PR, no amend needed just for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants