-
Notifications
You must be signed in to change notification settings - Fork 30k
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,console: add testing for monkeypatching of console stdio #26561
Conversation
lib/internal/console/constructor.js contains setters for console._stdout and console._stderr but these setters are not used in our tests or in Node.js core. (This is confirmed by our nightly coverage reports.) Add a test to check monkeypatching _stdout and _stderr on a console object. Version 2.6.9 of the very-popular npm module `debug` used this monkeypatching in its code. No other version did and they are now at version 4.something. It is not inconceivable that we would want to change the setters to throw rather than work. Given that this has seen use in the ecosystem, I'm inclined to leave the functionality in place.
The part where A user of I would therefore go ahead and deprecate or remove the setter (the latter only with a clean |
That might still be an argument for keeping it. Someone, at one point, found monkey-patching it useful. My opinion on this isn't a strong one, though, and I'd be fine with checking Gzemnid, changing the setter so that it throws, and running CITGM as an alternative. I worry that would stall, though, so I'd prefer to leave this open (and even land it) while that other process proceeds. |
lib/internal/console/constructor.js contains setters for console._stdout and console._stderr but these setters are not used in our tests or in Node.js core. (This is confirmed by our nightly coverage reports.) Add a test to check monkeypatching _stdout and _stderr on a console object. PR-URL: nodejs#26561 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Landed in 6913bd1 so we have a test for the setters. Will open a semver-major to change this behavior and we'll see how that goes. |
Actually, I think I'm not going to do that. We don't seem to prevent this sort of thing generally and there's no existing error code that I can find that seems to fit, suggesting we may not really do this at all? I'm not opposed to doing it, but it seems like a potentially significant departure and I'm inclined to leave well-enough alone. |
lib/internal/console/constructor.js contains setters for console._stdout and console._stderr but these setters are not used in our tests or in Node.js core. (This is confirmed by our nightly coverage reports.) Add a test to check monkeypatching _stdout and _stderr on a console object. PR-URL: nodejs#26561 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
lib/internal/console/constructor.js contains setters for console._stdout and console._stderr but these setters are not used in our tests or in Node.js core. (This is confirmed by our nightly coverage reports.) Add a test to check monkeypatching _stdout and _stderr on a console object. PR-URL: #26561 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
lib/internal/console/constructor.js contains setters for console._stdout and console._stderr but these setters are not used in our tests or in Node.js core. (This is confirmed by our nightly coverage reports.) Add a test to check monkeypatching _stdout and _stderr on a console object. PR-URL: #26561 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
lib/internal/console/constructor.js contains setters for console._stdout
and console._stderr but these setters are not used in our tests or in
Node.js core. (This is confirmed by our nightly coverage reports.)
Add a test to check monkeypatching _stdout and _stderr on a console
object.
Version 2.6.9 of the very-popular npm module
debug
used thismonkeypatching in its code. No other version did and they are now at
version 4.something. It is not inconceivable that we would want to
change the setters to throw rather than work. Given that this has seen
use in the ecosystem, I'm inclined to leave the functionality in place.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes