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: increase strictness for test-trace-event #11065

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 29, 2017

Change test-trace-event such that it checks that all expected values are
within the same trace object rather than scattered across multiple trace
objects.

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

test

@Trott Trott added the test Issues and PRs related to the tests. label Jan 29, 2017
@mscdex mscdex added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Jan 29, 2017
@Trott Trott changed the title test: increase strictenss for test-trace-event test: increase strictness for test-trace-event Jan 29, 2017
Change test-trace-event such that it checks that all expected values are
within the same trace object rather than scattered across multiple trace
objects.
Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

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

LGTM

if (trace.cat !== 'v8')
return false;
if (trace.name !== 'V8.ScriptCompiler')
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer to condense these but definitely not necessary to do so:

e.g.

return trace.pid === proc.pid &&
           trace.cat === 'v8' &&
           trace.name === 'V8.ScriptCompiler';

Copy link
Member Author

@Trott Trott Jan 31, 2017

Choose a reason for hiding this comment

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

I think changing each to an assert.strictEqual() would be even better because it gives descriptive information about exactly why the throw isn't matching. Otherwise, you kind of have to figure it out.

Copy link
Member Author

Choose a reason for hiding this comment

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

D'oh! No, I'm wrong, never mind. I thought this was inside an assert.throws() and not an Array.prototype.some(). Ignore my previous comment.

@jasnell
Copy link
Member

jasnell commented Jan 31, 2017

@Trott
Copy link
Member Author

Trott commented Feb 1, 2017

FreeBSD failure due to a stuck process and unrelated.

FreeBSD CI again: https://ci.nodejs.org/job/node-test-commit-freebsd/6796/

@Trott
Copy link
Member Author

Trott commented Feb 2, 2017

FreeBSD CI again again: https://ci.nodejs.org/job/node-test-commit-freebsd/6816/

Trott added a commit to Trott/io.js that referenced this pull request Feb 2, 2017
Change test-trace-event such that it checks that all expected values are
within the same trace object rather than scattered across multiple trace
objects.

PR-URL: nodejs#11065
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Feb 2, 2017

Landed in 773cdc3

@Trott Trott closed this Feb 2, 2017
@italoacasas
Copy link
Contributor

After #11106 land we can include this on v7.x-staging

targos pushed a commit to targos/node that referenced this pull request Mar 1, 2017
Change test-trace-event such that it checks that all expected values are
within the same trace object rather than scattered across multiple trace
objects.

PR-URL: nodejs#11065
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 1, 2017
Change test-trace-event such that it checks that all expected values are
within the same trace object rather than scattered across multiple trace
objects.

PR-URL: nodejs#11065
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas italoacasas mentioned this pull request Mar 1, 2017
@Trott
Copy link
Member Author

Trott commented Mar 3, 2017

This is dependent on #9304 which is semver-minor so (if I'm understanding correctly) should not land on the LTS branches. Adding dont-land-on labels. /cc @MylesBorins to confirm that I am Doing This Right™.

@Trott Trott deleted the matchingTraces branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants