Monkeypatch console.* calls on CI if we are asked to - #11897
Monkeypatch console.* calls on CI if we are asked to#11897Kartik Raj (karrtikr) merged 13 commits into
Conversation
b6554a0 to
6f6de02
Compare
0ca24d8 to
0be67ed
Compare
Eric Snow (ericsnowcurrently)
left a comment
Eric Snow (ericsnowcurrently)
left a comment
There was a problem hiding this comment.
This is great work, Kartik Raj (@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). 🙂
f5837ac to
3295936
Compare
Eric Snow (ericsnowcurrently)
left a comment
Eric Snow (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!
2ab66e5 to
e245f0e
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
7df168d
into
microsoft:logging-changes-and-drop-old-debugger
* 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
3 participants
Eric Snow (@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).