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

allowConsoleError should capture async errors #15609

Closed
lannka opened this issue May 25, 2018 · 6 comments
Closed

allowConsoleError should capture async errors #15609

lannka opened this issue May 25, 2018 · 6 comments

Comments

@lannka
Copy link
Contributor

lannka commented May 25, 2018

The following code illustrate the problem, where we expect a console error, but it actually might leak to outside of this test.

it('abc', () => {
  allowConsoleError(() => {
     asyncFuncThatPrintsConsoleError();
  });
})

function asyncFuncThatPrintsConsoleError() {
  setTimeout(() => {
     console.error('errr!!!')
  }, 1);
}
@lannka
Copy link
Contributor Author

lannka commented May 25, 2018

one idea is to make allowConsoleError more like expectConsoleError, which takes a regex pattern for expected console error msg. It returns a promise that resolves only when such a console error is found, or timed out.

it('abc', () => {
  asyncFuncThatPrintsConsoleError();
  return expectConsoleError('errr!!!');
})

@rsimha
Copy link
Contributor

rsimha commented May 25, 2018

@lannka Thanks for bringing this up.

In your suggested fix, what happens if a test that generates (sync or async) console errors simply fails to call expectConsoleError? We don't want the test to just pass (this was the original problem we were trying to fix). What mechanism will cause it to fail?

@lannka
Copy link
Contributor Author

lannka commented May 25, 2018

In your suggested fix, what happens if a test that generates (sync or async) console errors simply fails to call expectConsoleError?

We still stub console.error, and capture every message in a queue. the expectConsoleError() call removes message from the queue. We fail the test if we have a non-empty queue at the end.

@rsimha
Copy link
Contributor

rsimha commented May 25, 2018

Hmm. I want to make sure this doesn't place an undue burden on each test to explicitly declare which errors it expects twice.

So a test like this...

expect(() => {
  funcThatCallsConsoleError();
}).to.throw('Some error message');

... can be written like this today...

allowConsoleError(() => {
  expect(() => {
    funcThatCallsConsoleError();
  }).to.throw('Some error message');
});

... but with the new suggestion, might have to be re-written like this...

expect(() => {
  funcThatCallsConsoleError();
}).to.throw('Some error message');
expectConsoleError('Some error message');

I'll dig in to this some more and see if we can get the best of both worlds, where existing usage of allowConsoleError for sync errors is preserved, but async use can be supported with something like expectAsyncConsoleError.

@rsimha
Copy link
Contributor

rsimha commented May 25, 2018

For a temporary workaround, see what #15606 does

@rsimha
Copy link
Contributor

rsimha commented May 26, 2018

I ended up implementing more or less what @lannka suggested in #15621. Here is a usage example:

it('abc', () => {
  expectAsyncConsoleError('Foo');
  expectAsyncConsoleError('Bar');
  func();
})

If func() doesn't result in calls to console.error('Foo') and console.error('Bar'), or if it does result in a call to console.error('Baz), the test will fail.

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

No branches or pull requests

2 participants