Use console-testing-library to get consistent snapshots

[kevin940726]

As mentioned in #271 (comment), console-testing-library is a simple package I built to help test console output more align to what users see.

It's new, so it's probably not stable for every cases, but I think it's quite stable for our simple use case.

Under the hood, it uses new Console() as in #257 did, and also uses pretty-format from jest to generate consistent output across different node versions.

[netlify]

Deploy preview for redux-starter-kit-docs ready!

Built with commit 9484114

https://deploy-preview-277--redux-starter-kit-docs.netlify.com

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9484114:

Sandbox	Source
stupefied-meadow-xg23d	Configuration
tender-butterfly-quhzv	Configuration
rsk-github-issues-example	Configuration

[phryneas]

Now both CIs work nicely - thank you very much!

[markerikson]

Hmm. I just pulled master, and the serializable test file is failing with:

 FAIL  src/serializableStateInvariantMiddleware.test.ts
  ● Test suite failed to run

    Call retries were exceeded

      at ChildProcessWorker.initialize (node_modules/jest-worker/build/workers/ChildProcessWorker.js:193:21)

Any idea what's going on here? Does this work on Windows?

[markerikson]

It specifically seems to be the test on line 83, "'Should log an error when a non-serializable action is dispatched'".

If I skip that one, everything else passes:

 PASS  src/serializableStateInvariantMiddleware.test.ts
  findNonSerializableValue
    √ Should return false if no matching values are found (5ms)
    √ Should return a keypath and the value if it finds a non-serializable value (3ms)
    √ Should return the first non-serializable value it finds (1ms)
    √ Should return a specific value if the root object is non-serializable (1ms)
    √ Should accept null as a valid value (2ms)
  serializableStateInvariantMiddleware
    √ Should log an error when a non-serializable value is in state (13ms)
    √ Should use the supplied isSerializable function to determine serializability (1ms)
    √ should not check serializability for ignored action types (1ms)
    ○ skipped Should log an error when a non-serializable action is dispatched
    consumer tolerated structures
      √ Should log an error when a non-serializable value is nested in state (1ms)
      √ Should use consumer supplied isSerializable and getEntries options to tolerate certain structures (2ms)

If I include it, the test file fails with no actual error output:

 RUNS  src/serializableStateInvariantMiddleware.test.ts
npm ERR! Test failed.  See above for more details.

I want to get 1.2.0 out, but this needs to be resolved first. Can one of you take a look at this?

[kevin940726]

@markerikson I’ve seen similar result on older version of node 13, I’ve resolved it by just upgrading node to the latest version. What version are you on?

[markerikson]

13.2, looks like.

What about the lib makes it dependent on a specific Node version that way, and why aren't all the other tests failing like that too?

[kevin940726]

TBH, I just assumed it to be a node bug, I haven’t looked into the root cause. Related issue nodejs/node#30730, jestjs/jest#8769

[markerikson]

Uh.... I just commented out the entire body of that test, and the test run still dies the same way. But, if I .skip() the test, all tests in that file pass.

wat.

[kevin940726]

Looks like node 13.2 specifically is the only node version which has this problem, and it’s somehow unpredictable, at least in user-land

[markerikson]

I'll try upgrading Node when I'm free later.

[phryneas]

Currently I assume that this is some kind of memory leak in https://github.com/kevin940726/console-testing-library/blob/master/index.js#L177-L194 that only appears on node 13.2 with a minimum amount of tests being run - but I don't have a system where I could reproduce it. @kevin940726 , could you keep investigating please?

[kevin940726]

yeah sure! but we’re around 3 am here, let me do it tmr 😅

[phryneas]

Sure :D get some sleep!

[kevin940726]

Did some digging, unfortunately nothing new. Found out that it's a V8 bug, which has been fixed in Node. The bug has been happening in many tests, but what exactly triggers it is still unknown to me.

I don't think it'd be a blocker since it's just a dev dep, and we could just upgrade our node version to the latest. However, I will keep an eye on this and hope that we don't introduce any potential bugs.

[phryneas]

Well, given it's a known (and fixed) node bug that leads to a real segfault, that would be fine with me personally. Depends on what @markerikson thinks about it.
(By the way I could reproduce it in Linux by using 13.2)

One thing though: After reviewing this a second time, I'm not too fond that this automatically does a beforeEach and afterEach on import.
That way, when writing a test all my console.logs I'm doing just to validate a value or something are lost and it's not clear why.
I would prefer it if these beforeEach and afterEach were in our test, for transparency. Or a call to mockConsoleInAllTests or something similar.

PS: maybe you could add a line to your lib that detects 13.2 and just writes a warning to the console and opts out of doing anything if that's the case? It's better having some failing tests and a readable warning than a segfault.

[kevin940726]

Yeah the manual cleaning up makes sense to me. Thanks for the suggestion!

The problem is that I’m not totally sure what causes the problem, from my understanding so far is it’s a bug related to global inline cache. A warning would be nice but at the same time it might seems overly aggressive. FWIW it never happens in the test of the lib itself 🤔

[markerikson]

I tried pulling master and was still seeing the same error with the serializable middleware, but it did indeed go away after I upgraded from Node 13.2.0 to 13.5.0. shrug