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

gz test is permafailing on node v11.12.0 #1879

Closed
gregtatum opened this issue Mar 21, 2019 · 4 comments
Closed

gz test is permafailing on node v11.12.0 #1879

gregtatum opened this issue Mar 21, 2019 · 4 comments
Labels
tests Anything related to tests

Comments

@gregtatum
Copy link
Member

It's not failing on v11.10.0.

/cc @julienw any ideas?

 FAIL  src/test/unit/gz.test.js (5.901s)
  ● utils/gz › compresses and decompresses properly

    Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.

      21 |
      22 | describe('utils/gz', function() {
    > 23 |   it('compresses and decompresses properly', async () => {
         |   ^
      24 |     const clearText = '42';
      25 |     const gzipedData = await compress(clearText);
      26 |     expect(gzipedData).toBeInstanceOf(Uint8Array);

      at new Spec (node_modules/jest-jasmine2/build/jasmine/Spec.js:101:22)
      at Suite.it (src/test/unit/gz.test.js:23:3)
      at Object.describe (src/test/unit/gz.test.js:22:1)
@gregtatum gregtatum added the tests Anything related to tests label Mar 21, 2019
@julienw
Copy link
Contributor

julienw commented Mar 22, 2019

As a quick note, I tried running in v11.11 as well, but this release has another issue that got fixed in v11.12 in nodejs/node#26488 (comment).

There has been quite some changes in v11.11 about the worker implementation, that's why I wanted to check this version.

I believe this is what broke for us: nodejs/node#26082
This fixes something that I actually had to work around in my implementation, but as a result this now breaks the workaround :D

Here is what I think breaks:

postMessage(
message: mixed,
transfer?: Array<ArrayBuffer | MessagePort | ImageBitmap>
) {
this._instance.postMessage({ data: message }, transfer);
}
onMessage = (message: Object) => {
if (this.onmessage) {
this.onmessage(new MessageEvent('message', { data: message }));
}
};

As you can see, we create the { data: message } structure "manually", while it looks like from the linked PR that nodejs takes care of this now.

There are 3 options to fix this:

  1. do not fix this, and support only latest LTS (that is v10), that's run in our CI. We'll change this when bumping to the next LTS. We's still have to add some code to tell the user that we don't support other versions.
  2. fix this and support only node > v11.12.
  3. fix this by trying to detect in which case we are, and support both cases.

My preference is either 1 or 2. Maybe a slight preference for 2, because that's the easiest solution.

What do you think?

@julienw
Copy link
Contributor

julienw commented Mar 22, 2019

This fixes it on node v11.12:

diff --git a/src/utils/__mocks__/worker-factory.js b/src/utils/__mocks__/worker-factory.js
index 63934de0..3da07c32 100644
--- a/src/utils/__mocks__/worker-factory.js
+++ b/src/utils/__mocks__/worker-factory.js
@@ -18,17 +18,17 @@ class NodeWorker {
     worker.on('error', this.onError);
     this._instance = worker;
   }
 
   postMessage(
     message: mixed,
     transfer?: Array<ArrayBuffer | MessagePort | ImageBitmap>
   ) {
-    this._instance.postMessage({ data: message }, transfer);
+    this._instance.postMessage(message, transfer);
   }
 
   onMessage = (message: Object) => {
     if (this.onmessage) {
       this.onmessage(new MessageEvent('message', { data: message }));
     }
   };

So I see that the first postMessage automatically adds the { data: XXX } structure, but the one we receive in onmessage still doesn't have it. I asked about it in nodejs/node#26082 (comment), it they plan to change this in the future we may want to wait for it too.

@gregtatum
Copy link
Member Author

We could replace the test with #1730 so that we are not relying on experimental features.

@julienw
Copy link
Contributor

julienw commented Mar 22, 2019

I think this won't stay experimental for a long time now and this provides the right semantics for Worker, so it has some value.
I understand the problems of relying on experimental features though.

Supporting both node versions shouldn't be so hard. But it would also make it easier if we'd stick to latest node LTS instead of using node current :)

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

No branches or pull requests

2 participants