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: expanded assertions for console.timeEnd() output #17368

Closed
wants to merge 1 commit into from

Conversation

nikniv
Copy link
Contributor

@nikniv nikniv commented Nov 28, 2017

Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

Contributes towards #14544.

Refs: #14643

Checklist
  • make -j4 test (UNIX) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, console

Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve nodejs#14544 but will
not address the whole thing.

Refs: nodejs#14643
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 28, 2017
@mscdex mscdex added the console Issues and PRs related to the console subsystem. label Nov 28, 2017
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 30, 2017
@addaleax
Copy link
Member

@addaleax
Copy link
Member

addaleax commented Dec 1, 2017

Landed in 5282f96

Thanks for the PR! 🎉 ✨

@addaleax addaleax closed this Dec 1, 2017
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 1, 2017
addaleax pushed a commit that referenced this pull request Dec 1, 2017
Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve #14544 but will
not address the whole thing.

PR-URL: #17368
Refs: #14643
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve #14544 but will
not address the whole thing.

PR-URL: #17368
Refs: #14643
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve #14544 but will
not address the whole thing.

PR-URL: #17368
Refs: #14643
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Test fails on v8.x with:

=== release test-console ===                                                   
Path: parallel/test-console
label: 0.032ms
foo
foo bar
foo bar hop
{ slashes: '\\\\' }
inspect
foo
foo bar
foo bar hop
{ slashes: '\\\\' }
inspect
{ foo: 'bar', inspect: [Function: inspect] }
{ foo: 'bar', inspect: [Function: inspect] }
{ foo: [Object] }
{ foo: { bar: [Object] } }
label: 0.008ms
__proto__: 0.004ms
constructor: 0.004ms
hasOwnProperty: 0.004ms
label1: 0.014ms
label2: 0.091ms
label3: 0.130ms
foo
foo bar
foo bar hop
{ slashes: '\\\\' }
inspect
foo
foo bar
foo bar hop
{ slashes: '\\\\' }
inspect
Trace: This is a {"formatted":"trace"} 10 foo
    at Object.<anonymous> (/build/gib/node/test/parallel/test-console.js:92:9)
    at Module._compile (module.js:643:30)
    at Object.Module._extensions..js (module.js:654:10)
    at Module.load (module.js:556:32)
    at tryModuleLoad (module.js:499:12)
    at Function.Module._load (module.js:491:3)
    at Function.Module.runMain (module.js:684:10)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:608:3
/build/gib/node/test/parallel/test-console.js:147
assert.ok(/^: \d+\.\d{3}ms$/.test(strings.shift().trim()));
                                                 ^

TypeError: Cannot read property 'trim' of undefined
    at Object.<anonymous> (/build/gib/node/test/parallel/test-console.js:147:50)
    at Module._compile (module.js:643:30)
    at Object.Module._extensions..js (module.js:654:10)
    at Module.load (module.js:556:32)
    at tryModuleLoad (module.js:499:12)
    at Function.Module._load (module.js:491:3)
    at Function.Module.runMain (module.js:684:10)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:608:3
Command: out/Release/node /build/gib/node/test/parallel/test-console.js

@nikniv
Copy link
Contributor Author

nikniv commented Dec 20, 2017

@gibfahn, not sure if I understand correctly. Is the underlying PR #14643 a part of v8.x already? If not, the tests that I have added in this PR to cover the changes will fail.

I have read through the backporting guide. If I got that right, in case it is decided to go ahead with the backport, then I am supposed to initiate the backport PR according to the defined process, correct?

@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

@gibfahn, not sure if I understand correctly. Is the underlying PR #14643 a part of v8.x already? If not, the tests that I have added in this PR to cover the changes will fail.

Ahh, my mistake, so this tests functionality added in #14643, which is semver-major and thus won't go into v8.x. I'll add the dont-land-on-v8.x label.

I have read through the backporting guide. If I got that right, in case it is decided to go ahead with the backport, then I am supposed to initiate the backport PR according to the defined process, correct?

Yes, that would be correct if this did need a backport (which as you point out it doesn't). Thanks for clarifying!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants