Node: Legit CITGM failure?

[BridgeAR]

ws fails on OS-X and on AIX on citgm all the time and this was originally reported in 2017 here: https://github.com/websockets/ws/issues/1118

Example failure: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1418/nodes=osx1010/testReport/junit/(root)/citgm/ws_v5_1_1/

   262 passing (4s)
   1 failing
   1) PerMessageDeflate
        #compress and #decompress
          doesn't call the callback twice when `maxPayload` is exceeded:
      Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

It seemed this the failures started after adding the destroy functions. This was meant to be fixed later on but it seems like that fix did indeed not fix the issue or it was introduced later on again / a similar issue was introduced.

@nodejs/streams @mcollina @lpinca PTAL

I am not sure if this is an issue with Node.js core or with the ws module.

[gireeshpunathil]

I can debug these failures, but would appreciate if someone who knows CIGTM can tell me what test it is doing - is it just npm install ws and npm test ? or something else / more?

/cc @nodejs/platform-macos @nodejs/platform-aix

[Trott]

@gireeshpunathil I think it downloads the tarball for current ws (5.1.1 as of this writing), untars it, goes into the source directory, does npm install (so that it has everything it needs to run tests), and does npm test.

[gireeshpunathil]

this is upto where I was able to reach, I don't think this is the route CIGTM took, so stopping here and see if @lpinca has better insights.

npm test

> [email protected] test /Users/gireeshpunathil/Desktop/node_modules/ws/rec/package
> eslint . && nyc --reporter=html --reporter=text mocha test/*.test.js


Oops! Something went wrong! :(

ESLint: 4.19.1.
ESLint couldn't find a configuration file. To set up a configuration file for this project, please run:

    eslint --init

eslint --init

? How would you like to configure ESLint? Use a popular style guide
? Which style guide do you want to follow? 
  Google 
  Airbnb 
❯ Standard 
#eslint --init
? How would you like to configure ESLint? Inspect your JavaScript file(s)
? Which file(s), path(s), or glob(s) should be examined? /Users/gireeshpunathil/Desktop/node_modules/ws/
? What format do you want your config file to be in? JavaScript
? Are you using ECMAScript 6 features? Yes
? Are you using ES6 modules? Yes
? Where will your code run? Node
? Do you use JSX? No

(/Users/gireeshpunathil/Desktop/node_modules/ws/node_modules/mkdirp/bin/cmd.js:13:5) Parsing error: 'return' outside of function
...

[Trott]

@gireeshpunathil Sorry! I should have been more specific. You need to download the source tarball, not the release tarball. Otherwise it won't have things like the ESLint config file. Try the tarball at https://github.com/websockets/ws/archive/5.1.1.tar.gz.

[gireeshpunathil]

no problem, thanks - now I get exactly what is caught in the CI console:

  262 passing (4s)
  1 failing

  1) PerMessageDeflate
       #compress and #decompress
         doesn't call the callback twice when `maxPayload` is exceeded:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

will see what happened inside.

[lpinca]

Yes, that failure is legit on Node.js 10. That test was calling zlib.flush() after calling zlib.close() and that no longer works in Node.js 10, not sure why it was working in older versions.

I fixed it in https://github.com/websockets/ws/commit/bb9c21c1aff2949b648747825f1b482d389fd2ae but didn't publish a new version yet.

[gireeshpunathil]

thanks @lpinca ! I was breaking my head on why the compress way not throwing error while decompress does etc.

I hope we can close this out then.

[apapirovski]

Sounds like we can close this. Feel free to reopen if I misunderstood something and this still needs attention in Node.js core.