ValentinH / jest-fail-on-console

Utility to make jest tests fail when console.error() or any other methods are used
MIT License
141 stars 20 forks source link

Console messages emitted from Jest's `beforeAll` hook do not cause `failOnConsole` to fail the test #33

Open rszalski opened 1 year ago

rszalski commented 1 year ago

HI! First of all, thank you for this library, it was a godsend for us when cleaning up our messy unit test report :)

I think I have found a bug that caught us off guard, though. It seems that if the console is used in beforeAll (it might be other Jest hooks too, I just have not confirmed) it doesn't cause the tests to fail.

Our configuration:

failOnConsole({
  shouldFailOnWarn: true,
  shouldFailOnError: true,
  shouldFailOnAssert: true,
  shouldFailOnDebug: true,
  shouldFailOnInfo: true,
  shouldFailOnLog: true,
  silenceMessage: m => {
    // This error is removed in React 18 because it is misleading, so suppressing until we upgrade: https://github.com/reactwg/react-18/discussions/82
    if (m.includes("Can't perform a React state update on an unmounted component.")) {
      return true;
    }
    return false;
  }
});

An example which will emit the following error:

Screenshot 2023-02-15 at 14 59 34
  beforeEach(() => {
    jest.useFakeTimers();
  });

  // `beforeAll` is the cause of the warning, it should be `afterEach` but this illustrates the issue
  beforeAll(() => {
    // Emitted here, but test PASS
    jest.runOnlyPendingTimers();
    jest.useRealTimers();
  });

  it('Emits a warning which correctly fails the test run', () => {
   // This is the same sequence of commands and they also result in a Jest warning. This time tests will FAIL
    jest.runOnlyPendingTimers();
    jest.useRealTimers();
    jest.useFakeTimers();
  });

Please take a look at this code sandbox: https://codesandbox.io/s/jest-fail-on-console-hooks-7rut79?file=/src/index.test.js Unfortunately, even though Codesandbox supports Jest and the tests run, it doesn't seem to allow us to use any of the fake-timer controlling features due to this bug: https://github.com/codesandbox/codesandbox-client/issues/513. Hopefully, this example is easy enough to reproduce locally.

ValentinH commented 1 year ago

Thanks for the detailed issue report.

Unfortunately, I'm not sure that we can do something about this 😬 Indeed, we are using beforeEach/afterEach to setup/teardown this plugin logic for each test: https://github.com/ValentinH/jest-fail-on-console/blob/main/index.js#L85-L99

As far as I know, they run after beforeAll so too late in your case.

Conceptually, I'm not sure we could make a test fail because this run before all tests. Therefore, which test should be failed in that case?

That being said, we could at least update the README to explain this limitation.

rszalski commented 1 year ago

@ValentinH in this case I would be ok with not knowing which test failed. The most important thing would be that it fails at all. But I agree it might not be possible with the current architecture.

flakey-bit commented 1 year ago

A related use-case - we're relying on silenceMessage to silence some annoying warning mesages. Sometimes, those warning messages are emited during beforeAll - at the moment, jest-fail-on-console is unable to silence those messages for us.

It would be great if that aspect (at least) could be fixed 🤞

ValentinH commented 1 year ago

Silencing might work if we also register our interceptors in a beforeAll call. Today, we only do it in a beforeEach. Would you be willing to test this and submit a PR if it works?

However, I'm still not sure if failing tests if we see that a non-silenced console is called could be done for the reasons I shared above.

flakey-bit commented 1 year ago

@ValentinH registering the inceptors in beforeAll did help 👍 - see https://github.com/ValentinH/jest-fail-on-console/pull/39

I had issues running the tests locally on main after a yarn install (i.e. before making changes) so I'm unable to check whether the changes impact any tests.

yarn run v1.22.5
$ jest tests/index.test.js
 FAIL  tests/index.test.js
  jest-fail-on-console
    × errors when console.error() is called (27 ms)
    × does not error when console.error() is called but shouldFailOnError is false (23 ms)
    × does not error when console.error() is called and skip test by test file path returns true (23 ms)
    × does not error when console.error() is called and skip test by test name returns true (22 ms)
    × errors when console.assert() is called with a failing assertion (22 ms)
    × does not error when console.assert() is called with a passing assertion (22 ms)
    × does not error if message is silenced with `silenceMessage` (23 ms)
    × does not error if message is silenced with `silenceMessage` based on console method (23 ms)
    × does not error if message is silenced with `silenceMessage` based on group context (23 ms)
    × does not error if message is silenced with `silenceMessage` based on nested group context (23 ms)

  ● jest-fail-on-console › errors when console.error() is called

    expect(received).toEqual(expected) // deep equality

    Expected: StringContaining "Expected test not to call console.error()."
    Received: "'.' is not recognized as an internal or external command,·
    operable program or batch file.·
    "

      21 |     const { stderr } = await runFixture('error')
      22 |
    > 23 |     expect(stderr).toEqual(expect.stringContaining('Expected test not to call console.error().'))
         |                    ^
      24 |   })
      25 |
      26 |   it('does not error when console.error() is called but shouldFailOnError is false', async () => {

      at Object.<anonymous> (tests/index.test.js:23:20)

  ● jest-fail-on-console › does not error when console.error() is called but shouldFailOnError is false

    expect(received).toEqual(expected) // deep equality

    Expected: StringContaining "PASS tests/fixtures/error-disabled/index.test.js"
    Received: "'.' is not recognized as an internal or external command,·
    operable program or batch file.·
    "
ValentinH commented 1 year ago

Thank you for taking the time. That's unfortunate that the tests are not runnable, are you on Windows?

flakey-bit commented 1 year ago

Thank you for taking the time. That's unfortunate that the tests are not runnable, are you on Windows?

Yep

yarn --version
1.22.5
node --version
v18.13.0
ValentinH commented 1 year ago

Ok that's means that some code in the tests system are not compatible with Windows unfortunately.