test: improve test-async-hooks-http-parser-destroy #27319

Trott · 2019-04-19T22:11:26Z

Improve reporting in test-async-hooks-http-parser-destroy when failing.

Before, failures looked like this (edited slightly to conform to our
commit message 72-char line length restriction):

The expression evaluated to a falsy value:
assert.ok(destroyedIds.indexOf(createdAsyncId) >= 0)

Now, you get a slightly better idea of what's up. (Is it missing one ID?
More than one? All of them?):

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected ... Lines skipped

  [
    156,
...
    757,
-   761,
    765
  ]

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2019-04-19T22:12:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22572/

test/parallel/test-async-hooks-http-parser-destroy.js

nodejs-github-bot · 2019-04-20T00:16:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22574/

nodejs-github-bot · 2019-04-20T05:58:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22576/

BridgeAR · 2019-04-21T17:05:31Z

test/parallel/test-async-hooks-http-parser-destroy.js

+    if (createdIds.includes(asyncId)) {
+      // Work around https://github.com/nodejs/node/issues/26961.
+      // Remove the next line when #26961 is fixed.
+      if (!destroyedIds.includes(asyncId))


I would rather not include this part and instead mark this test as flaky. Otherwise we might forget about it and the test will silently work while it should fail.

The failure indicates that we have a bug somewhere that we should fix.

I see what you're saying and I'm definitely sympathetic. The problem is that with the changes in this PR, the test will basically fail 100% of the time. And I'm generally not OK with marking a test "flaky" as a workaround for a test that fails on every run. But the change is (IMO) a genuine improvement. It makes the checks more thorough (which is why it increases the failure rate dramatically).

I guess the solution everyone can support would be to fix the underlying bug.

Now that ece5073 landed, let's see if that fixes the remaining issues here and if I can remove this workaround.... Going to run CI with this and then again without it....

Looks like HTTPPARSER ids never show up now. I imagine that's unintended....

Oh, it totally was intentional. Guess I need to update the test to use HTTPINCOMINGMESSAGE or something like that. Or maybe the test can/should be removed entirely?

Probably the thing to do is to simply not filter the asyncIDs at all. If V8 compiling ever finishes for me locally, I'll try that out....

Alas, it did not appear to fix the bug. IDs show up in destroy without showing up in create. But I should at least update the test to use HTTPINCOMINGMESSAGE.

nodejs-github-bot · 2019-04-22T19:30:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22635/

nodejs-github-bot · 2019-04-23T00:01:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22646/

nodejs-github-bot · 2019-04-23T04:31:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22649/

Trott · 2019-04-23T13:03:10Z

Passes everywhere except when run in a worker (using tools/test.py --worker:

not ok 186 parallel/test-async-hooks-http-parser-destroy
  ---
  duration_ms: 5.423
  severity: fail
  exitcode: 1
  stack: |-
    
    events.js:173
          throw er; // Unhandled 'error' event
          ^
    AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
    + actual - expected ... Lines skipped
    
      [
        158,
    ...
        487,
    -   491,
    -   495,
    -   499,
    -   503,
    -   507
      ]
        at process.checkOnExit (/home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/parallel/test-async-hooks-http-parser-destroy.js:61:10)
        at process.emit (events.js:201:15)
    Emitted 'error' event at:
        at Worker.[kOnErrorMessage] (internal/worker.js:161:10)
        at Worker.[kOnMessage] (internal/worker.js:171:37)
        at MessagePort.<anonymous> (internal/worker.js:104:57)
        at MessagePort.emit (events.js:196:13)
        at MessagePort.onmessage (internal/worker/io.js:68:8)
  ...

Trott · 2019-04-23T13:03:44Z

Missing some destroy calls when run in a worker....

Improve reporting in test-async-hooks-http-parser-destroy when failing. Before, failures looked like this (edited slightly to conform to our commit message 72-char line length restriction): The expression evaluated to a falsy value: assert.ok(destroyedIds.indexOf(createdAsyncId) >= 0) Now, you get a slightly better idea of what's up. (Is it missing one ID? More than one? All of them?): AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal: + actual - expected ... Lines skipped [ 156, ... 757, - 761, 765 ]

Trott · 2019-05-03T18:40:47Z

Now that #26961 has landed, this can be unblocked/unstalled. I've rebased, removed the workaround for #26961, and I think this is ready to go? Re-reviews would be helpful. @addaleax @joyeecheung

Trott · 2019-05-03T20:56:35Z

Argh! Still fails with --worker....

Trott · 2019-05-03T21:17:39Z

OK, fixed for workers!

nodejs-github-bot · 2019-05-05T00:28:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22960/

Trott · 2019-05-05T04:19:50Z

Landed in 11e1e00

Improve reporting in test-async-hooks-http-parser-destroy when failing. Before, failures looked like this (edited slightly to conform to our commit message 72-char line length restriction): The expression evaluated to a falsy value: assert.ok(destroyedIds.indexOf(createdAsyncId) >= 0) Now, you get a slightly better idea of what's up. (Is it missing one ID? More than one? All of them?): AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal: + actual - expected ... Lines skipped [ 156, ... 757, - 761, 765 ] PR-URL: nodejs#27319 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Improve reporting in test-async-hooks-http-parser-destroy when failing. Before, failures looked like this (edited slightly to conform to our commit message 72-char line length restriction): The expression evaluated to a falsy value: assert.ok(destroyedIds.indexOf(createdAsyncId) >= 0) Now, you get a slightly better idea of what's up. (Is it missing one ID? More than one? All of them?): AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal: + actual - expected ... Lines skipped [ 156, ... 757, - 761, 765 ] PR-URL: #27319 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Trott added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. async_hooks Issues and PRs related to the async hooks subsystem. labels Apr 19, 2019

addaleax approved these changes Apr 19, 2019

View reviewed changes

test/parallel/test-async-hooks-http-parser-destroy.js Show resolved Hide resolved

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 20, 2019

joyeecheung approved these changes Apr 20, 2019

View reviewed changes

Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 20, 2019

BridgeAR reviewed Apr 21, 2019

View reviewed changes

Trott force-pushed the simplify-test branch from d76cff9 to d58ec1f Compare April 23, 2019 00:00

Trott force-pushed the simplify-test branch from d58ec1f to 2bc9310 Compare May 3, 2019 18:40

fixup! test: improve test-async-hooks-http-parser-destroy

d478ae8

This comment has been minimized.

Sign in to view

fixup! fixup! test: improve test-async-hooks-http-parser-destroy

9e5a8f4

This comment has been minimized.

Sign in to view

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 4, 2019

fixup! fixup! fixup! test: improve test-async-hooks-http-parser-destroy

4827486

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: improve test-async-hooks-http-parser-destroy #27319

test: improve test-async-hooks-http-parser-destroy #27319

Trott commented Apr 19, 2019

nodejs-github-bot commented Apr 19, 2019

nodejs-github-bot commented Apr 20, 2019

nodejs-github-bot commented Apr 20, 2019

BridgeAR Apr 21, 2019

Trott Apr 21, 2019

Trott Apr 22, 2019

Trott Apr 22, 2019

Trott Apr 22, 2019

Trott Apr 22, 2019

Trott Apr 22, 2019

nodejs-github-bot commented Apr 22, 2019

nodejs-github-bot commented Apr 23, 2019

nodejs-github-bot commented Apr 23, 2019

Trott commented Apr 23, 2019

Trott commented Apr 23, 2019

Trott commented May 3, 2019

This comment has been minimized.

Trott commented May 3, 2019 •

edited

Loading

Trott commented May 3, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented May 5, 2019

Trott commented May 5, 2019

test: improve test-async-hooks-http-parser-destroy #27319

test: improve test-async-hooks-http-parser-destroy #27319

Conversation

Trott commented Apr 19, 2019

Checklist

nodejs-github-bot commented Apr 19, 2019

nodejs-github-bot commented Apr 20, 2019

nodejs-github-bot commented Apr 20, 2019

BridgeAR Apr 21, 2019

Choose a reason for hiding this comment

Trott Apr 21, 2019

Choose a reason for hiding this comment

Trott Apr 22, 2019

Choose a reason for hiding this comment

Trott Apr 22, 2019

Choose a reason for hiding this comment

Trott Apr 22, 2019

Choose a reason for hiding this comment

Trott Apr 22, 2019

Choose a reason for hiding this comment

Trott Apr 22, 2019

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 22, 2019

nodejs-github-bot commented Apr 23, 2019

nodejs-github-bot commented Apr 23, 2019

Trott commented Apr 23, 2019

Trott commented Apr 23, 2019

Trott commented May 3, 2019

This comment has been minimized.

Trott commented May 3, 2019 • edited Loading

Trott commented May 3, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented May 5, 2019

Trott commented May 5, 2019

Trott commented May 3, 2019 •

edited

Loading