Monkeypatch console.* calls on CI if we are asked to#11897
Conversation
ericsnowcurrently
left a comment
There was a problem hiding this comment.
This is great work, @karrtikr. I think the approach you took is spot-on. I've left a few minor comments and noted one possible correctness issue. Otherwise, LGTM.
| delete config.console; | ||
| // This is a separate logger that matches our config but | ||
| // does not do any console logging. | ||
| configureLogger(monkeyPatchLogger, config); |
There was a problem hiding this comment.
This means monkeyPatchLogger and globalLogger will each have a file handler writing to the same file. That is probably fine, though you may want to make sure it isn't a problem on Windows. (IIRC, on Windows only one process may have a file open at a time. I may be wrong though.)
There was a problem hiding this comment.
We use console.* calls only in tests before the extension activation. globalLogger is used after extension activation, so these 2 shouldn't conflict.
There was a problem hiding this comment.
As long as we're sure about console.log(), etc. Just keep in mind that any 3rd-party module (or VS Code itself) may also be calling console.log() at any time after the extension activates. Of course, it's a non-issue if having two logging handlers for the same file doesn't break (on Windows).
FWIW, my only objective was to make sure this had been considered and wasn't a problem (rather than to say there was a problem). 🙂
ericsnowcurrently
left a comment
There was a problem hiding this comment.
Nice work! Other than the question about GH Actions and the previous comment about closures, LGTM. I'm approving the PR under the assumption that you will address both. Thanks!
|
Kudos, SonarCloud Quality Gate passed!
|
* Move console.* monkeypatching to src/test/testLogging.ts * Use monkeypatching in all tests launched using testBootstrap.ts * Use monkeypatching in single workspace and multiroot tests * News entry * Don't do monkeypatching for smoketests * Modify gulp task to not delete 'out/client/logging' directory * Undo smoke test ccheck * Import only from ./out/client/logging in test logger * Added comment * More corrections * Add comments * Correct smoke tests * Oops
* Move console.* monkeypatching to src/test/testLogging.ts * Use monkeypatching in all tests launched using testBootstrap.ts * Use monkeypatching in single workspace and multiroot tests * News entry * Don't do monkeypatching for smoketests * Modify gulp task to not delete 'out/client/logging' directory * Undo smoke test ccheck * Import only from ./out/client/logging in test logger * Added comment * More corrections * Add comments * Correct smoke tests * Oops
@ericsnowcurrently I've not done monkeypatching for smoke tests because
outdirectory isn't available while running it and we've to copy all of the logging code totestdirectory for it to work. Let me know if you think I should still do it, or if there's a better way.For #11896
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).