console: console.countReset() should emit warning

[domfarolino]

The Console Standard specifies that console.countReset()
should emit some type of a warning when given a label that
has no previous account associated with it. This PR brings
node's implementation of console.countReset() up-to-spec and
adds a test asserting that a warning is emitted.

Fixes: #20524

---

/cc @TimothyGu @devsnek

It would probably be worth adding more tests for console.countReset(), similarly to what #21312 is doing, but that can probably be take care of outside of this. I can file another issue if that seems reasonable.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[TimothyGu]

CI: https://ci.nodejs.org/job/node-test-pull-request/15727/

[domfarolino]

Rebased this + updated the console label used for warning-emitting tests from nolabel to noLabel (just a style decision). Hopefully CI passes this time.

[TimothyGu]

New CI: https://ci.nodejs.org/job/node-test-pull-request/15730/

[domfarolino]

Seems a couple things failed again, but I can't easily tell from the logs why. Thoughts?

[addaleax]

@domfarolino I wouldn’t worry about it, it’s highly unlikely that that failure is related.

Windows CI re-run to be sure: https://ci.nodejs.org/job/node-test-commit-windows-fanned/19196/

[domfarolino]

Looks like the latest Windows CI passed IIUC?

[jasnell]

CI looks good, this should be ready to land

[jasnell]

Landed in d4164ca

[targos]

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

[domfarolino]

Is it me you're asking? Is there a reason this'd need to be backported? I'm really not sure.

[targos]

It's usually more a question for collaborators, but you can answer too, if you like :)

What happened is that I tried to cherry-pick this commit to the v10.x-staging branch (it's the branch that we use to prepare releases of Node 10). The commit from this PR does not land cleanly because of merge conflicts so I asked if the change is relevant to Node 10 and if someone can prepare a PR to backport it.

I used a reply template and didn't look closely at the code. Since this added a new warning to an existing method, I think it has to be labeled semver-major, and cannot be released in Node 10.

[AyushG3112]

@targos a general question just for me to understand, why is adding a warning to an existing method semver-major? I don't think this can break anything.

[targos]

@AyushG3112 I can imagine users who want to make sure their apps don't emit warnings. But maybe I'm being too safe here.

process.on('warning', (w) => {
  throw w;
});

require('fs').promises; // or console.countReset('doesnotexist')

D:\Desktop>node test.js
(node:2148) ExperimentalWarning: The fs.promises API is experimental
D:\Desktop\test.js:3
  throw w;
  ^

ExperimentalWarning: The fs.promises API is experimental
    at Object.get (fs.js:1855:17)
    at Object.<anonymous> (D:\Desktop\test.js:7:14)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:266:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)