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

report: print common items first for readability #27367

Closed
wants to merge 1 commit into from

Conversation

gengjiawen
Copy link
Member

cc @richardlau

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. report Issues and PRs related to process.report. labels Apr 23, 2019
@gengjiawen gengjiawen changed the title report: print common item first for readability report: print common items first for readability Apr 23, 2019
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the timer example in the docs too? Otherwise LGTM.

@gengjiawen
Copy link
Member Author

Can you update the timer example in the docs too? Otherwise LGTM.

I am not sure which part. Can you post a link or snippet ? Thanks.

@richardlau
Copy link
Member

Can you update the timer example in the docs too? Otherwise LGTM.

I am not sure which part. Can you post a link or snippet ? Thanks.

GitHub makes it hard to link to snippets in markdown docs but the timer handle info around L193.

@gengjiawen
Copy link
Member Author

GitHub makes it hard to link to snippets in markdown docs but the timer handle info around L193.

I see. I will try to fix it after I watched Avengers: Endgame tonight 😎.

@gengjiawen
Copy link
Member Author

@richardlau Fixed.

@jasnell
Copy link
Member

jasnell commented Apr 25, 2019

Hmm... are there no tests that are verifying the actual output here? I know there's quite a bit that's non-deterministic but there should at least be some validation in the test. Changing the order of these these should break at least something in the tests :-)

@richardlau
Copy link
Member

Hmm... are there no tests that are verifying the actual output here? I know there's quite a bit that's non-deterministic but there should at least be some validation in the test. Changing the order of these these should break at least something in the tests :-)

We have a test that verifies the libuv handle information (test/report/test-report-uv-handles.js) but it is not affected by reordering as it uses JSON.parse() to parse the report and tests the parsed data.

@jasnell
Copy link
Member

jasnell commented Apr 25, 2019

Hmm... ok, I think that's definitely something we can improve. A basic message type test that checks the unparsed output would be worthwhile

@gengjiawen
Copy link
Member Author

Hmm... ok, I think that's definitely something we can improve. A basic message type test that checks the unparsed output would be worthwhile

Is there any example I can reference for output key sort ?

@gengjiawen
Copy link
Member Author

gengjiawen commented Apr 25, 2019

Now we have nested structure like this, looks like more hard to match 😭

{
      "localEndpoint": {
        "host": "0.0.0.0",
        "port": 56288
      },
      "remoteEndpoint": null,
      "sendBufferSize": 9216,
      "recvBufferSize": 786896,
      "fd": 33,
      "type": "udp",
      "is_active": true,
      "is_referenced": true,
      "address": "0x0000000103507bf0"
    }
}

@gengjiawen
Copy link
Member Author

gengjiawen commented Apr 25, 2019

Will try something like this in test-report-uv-handles

    {
      const get_libuv = /"libuv":\s\[([\s\S]*?)\]/gm;
      const get_handle_inner = /{([\s\S]*?),*?}/gm;
      const libuv_handles_str = get_libuv.exec(stdout)[1];
      const libuv_handles_array = libuv_handles_str.match(get_handle_inner);

      libuv_handles_array.map(i => {
        // exclude nested structure
        if (i.includes('type')) {
          const handle_keys = i.match(/(".*"):/gm);
          assert(handle_keys[0], 'type');
          assert(handle_keys[1], 'is_active');
        }
      });
    }

@gengjiawen gengjiawen force-pushed the node_report_output branch 2 times, most recently from d6bef39 to 005d602 Compare April 25, 2019 13:58
@gengjiawen
Copy link
Member Author

gengjiawen commented Apr 25, 2019

Not know why this test keep failed on my win10, so just let's travis run the tests ...

Debugger attached.
assert.js:87
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Process exited unexpectedly with code1
    at ChildProcess.<anonymous> (D:\Developer\node\test\report\test-report-uv-handles.js:88:12)
    at ChildProcess.<anonymous> (D:\Developer\node\test\common\index.js:373:15)
    at ChildProcess.emit (events.js:194:13)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:254:12)
Waiting for the debugger to disconnect..

@gengjiawen
Copy link
Member Author

@richardlau @jasnell I add related test and CI is green, can you review this again ? thanks.

@ZYSzys ZYSzys added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 27, 2019
@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Apr 27, 2019

Landed in 38f3526

@targos targos closed this Apr 27, 2019
targos pushed a commit that referenced this pull request Apr 27, 2019
PR-URL: #27367
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
targos pushed a commit that referenced this pull request Apr 27, 2019
PR-URL: #27367
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
@targos targos mentioned this pull request Apr 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. report Issues and PRs related to process.report.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants