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

test: simplify test-async-hooks-http-parser-destroy #26963

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 28, 2019

Simplify test-async-hooks-http-parser-destroy and improve it's reporting
when failing. (Also makes it easier to run on older versions of Node.js
because it doesn't rely on internal test modules that won't work there.)

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?):

Input A expected to strictly deep-equal input B:
+ expected - actual ... Lines skipped

  Set {
    1009,
...
    1025,
-   158,
-   159,
-   161,
-   162,
-   164,
-   165,
-   167,
-   168,
-   170,
...
+   159,
+   162,
+   165,
+   168,
+   171,
+   174,
+   177,
+   180,
+   183,

This test still fails as expected on 10.14.1 and passees on 10.14.2
(where the regression it tests for was fixed). (You will need to comment
on the require('../common'); line first but that's now the only change
you need to make this run in older versions.)

Refs: #26610

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

@Trott 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 Mar 28, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott Sadly, an error occurred when I tried to trigger a build. :(

@richardlau
Copy link
Member

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

Input A expected to strictly deep-equal input B:
+ expected - actual ... Lines skipped

  Set {
    1009,
...
    1025,
-   158,
-   159,
-   161,
-   162,
-   164,
-   165,
-   167,
-   168,
-   170,
...
+   159,
+   162,
+   165,
+   168,
+   171,
+   174,
+   177,
+   180,
+   183,

It would be nice if the assert output for sets could be improved. For example, in the quoted example 159, 162, 165, 168 are listed in both + expected and - actual. I rolled a custom assertion message in #26531 but maybe this can be fixed at the assert level?

@BridgeAR
Copy link
Member

@richardlau there is an open issue about this and it is on my TODO list. It is sadly just not an easy task. I need meta information about the structure of the inspected object. Otherwise it's not possible to reliable compare the correct objects.

As soon as I have that, I would also like to implement a proper diffing algorithm and I already checked a couple of papers and some code about that as well.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member Author

Trott commented Mar 29, 2019

The good news is this version of the test passes on Pi now. The bad news is it fails when run inside a worker. Investigating.

@Trott Trott added the wip Issues and PRs that are still a work in progress. label Mar 29, 2019
@Trott Trott force-pushed the refactor-http-parser-destroy branch from 68da51f to b340477 Compare March 29, 2019 05:55
@Trott
Copy link
Member Author

Trott commented Mar 29, 2019

At this point, I have to believe this is uncovering a real bug involving destroy handers in async_hooks when used with the http module. My hope is that someone fixes #26961 and that fixes this too.

@Trott Trott force-pushed the refactor-http-parser-destroy branch from b340477 to 69cdc43 Compare March 31, 2019 05:18
@Trott
Copy link
Member Author

Trott commented Mar 31, 2019

Rebased, force-pushed, no longer failing in a worker, but still seems to be flaky due to #26961 or related. (I added code to work around asyncIds being reused, but it appears that they also might not appear at all in the destroy handler, which causes the test to timeout.)

@Trott Trott removed the wip Issues and PRs that are still a work in progress. label Mar 31, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Simplify test-async-hooks-http-parser-destroy and improve it's reporting
when failing. (Also makes it easier to run on older versions of Node.js
because it doesn't rely on internal test modules that won't work there.)

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?):

    Input A expected to strictly deep-equal input B:
    + expected - actual ... Lines skipped

      Set {
        1009,
    ...
        1025,
    -   158,
    -   159,
    -   161,
    -   162,
    -   164,
    -   165,
    -   167,
    -   168,
    -   170,
    ...
    +   159,
    +   162,
    +   165,
    +   168,
    +   171,
    +   174,
    +   177,
    +   180,
    +   183,

This test still fails as expected on 10.14.1 and passees on 10.14.2
(where the regression it tests for was fixed). (You will need to comment
on the `require('../common');` line first but that's now the only change
you need to make this run in older versions.)

Refs: nodejs#26610
@Trott Trott force-pushed the refactor-http-parser-destroy branch from 69cdc43 to 079d054 Compare April 19, 2019 17:56
@Trott Trott closed this Apr 19, 2019
@Trott Trott deleted the refactor-http-parser-destroy branch January 13, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants