-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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
Conversation
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.
There was a problem hiding this 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; |
There was a problem hiding this comment.
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';
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
FreeBSD failure due to a stuck process and unrelated. FreeBSD CI again: https://ci.nodejs.org/job/node-test-commit-freebsd/6796/ |
FreeBSD CI again again: https://ci.nodejs.org/job/node-test-commit-freebsd/6816/ |
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>
Landed in 773cdc3 |
After #11106 land we can include this on |
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>
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>
This is dependent on #9304 which is |
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), orvcbuild test
(Windows) passesAffected core subsystem(s)
test