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

We don't assert on warnings for tests that throw #12428

Closed
gaearon opened this issue Mar 22, 2018 · 2 comments
Closed

We don't assert on warnings for tests that throw #12428

gaearon opened this issue Mar 22, 2018 · 2 comments

Comments

@gaearon
Copy link
Collaborator

gaearon commented Mar 22, 2018

I think this shouldn't pass:

    expect(() => {
      expect(() => {
        throw new Error('haha') // no warning
      }).toWarnDev(
        'nope',
      );
    }).toThrow('haha');

but it does.

@bvaughn
Copy link
Contributor

bvaughn commented Mar 26, 2018

The reason the above test fails is because of this check:

// Any unexpected Errors thrown by the callback should fail the test.
// This should take precedence since unexpected errors could block warnings.
if (caughtError) {
throw caughtError;
}

As PR #12081 mentions:

While writing tests for unsafe async warnings, I noticed that in certain cases, errors were swallowed by the toWarnDev matcher and resulted in confusing test failures. For example, if an error prevented the code being tested from logging an expected warning- the test would fail saying that the warning hadn't been logged rather than reporting the unexpected error. I think a better approach for this is to always treat caught errors as the highest-priority reason for failing a test.

I'm not sure of the best way to handle this. If there was a way to detect this nesting combination (toWarnDev inside of a toThrow) we could explicitly error saying it's not supported, but I don't think that's possible?

@bvaughn
Copy link
Contributor

bvaughn commented Mar 26, 2018

Maybe I could add a lint rule to warn about the above combo?

Update: This might be trickier than I thought, since it seems like we have 3 tests that don't work if the nesting order is reversed. (When we replay failed work, the warning is logged a second time- even though we only expect it once.)

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