bootstrap: adds exception handling for profiler bootstrap

[shobhitchittora]

The bootstrapping of profiler failed the script evaluation when inspector is disabled. Adding a try-catch block to handle that and emit a warning.

Fixes: #29542

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

[shobhitchittora]

@addaleax The warning message can be better here. Any suggestions? Any straightforward way to write a test for this. Thinking of emitting the warning from child to parent and then asserting that.

[devsnek]

how about "The inspector is disabled, coverage could not be collected"

[addaleax]

@addaleax The warning message can be better here. Any suggestions?

I think something along @devsnek’s suggestion is fine.

Any straightforward way to write a test for this. Thinking of emitting the warning from child to parent and then asserting that.

Yeah, spawning a child process that emits this warning and then checking its output would be the easiest way to test this 👍

[shobhitchittora]

Both
./node ./test/parallel/test-coverage-with-inspector-disabled.js
and
python tools/test.py test/parallel/test-coverage-with-inspector-disabled.js
work fine for me on local, but the test is fails in CI. I might be missing something here. Is the child spawned correctly in the test added?

[addaleax]

Both
./node ./test/parallel/test-coverage-with-inspector-disabled.js
and
python tools/test.py test/parallel/test-coverage-with-inspector-disabled.js
work fine for me on local, but the test is fails in CI. I might be missing something here. Is the child spawned correctly in the test added?

I might be wrong, but is there a chance it’s working for you locally because you locally compiled a version of Node.js that has the inspector disabled, but Travis CI builds the default version with inspector enabled?

I think it might make sense to add something like if (process.features.inspector) common.skip('Inspector enabled'); or something like it to the beginning of the test?

[shobhitchittora]

@addaleax Can this be closed and merged now?

[addaleax]

@shobhitchittora Yeah, once CI is green with the latest changes this should be good to go 👍

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[Trott]

@shobhitchittora Can you rebase to get rid of the conflict?

[Trott]

@shobhitchittora Can you rebase to get rid of the conflict?

Never mind. I did it myself. At least one re-review would be good. @addaleax @joyeecheung

[nodejs-github-bot]

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

[addaleax]

Landed in fdd5d4a 🎉 Thanks for the PR!

[shobhitchittora]

Thanks all for merging this.