fix(rest): log unexpected errors to console #1058
Conversation
bajtos
added
bug
developer-experience
packages/build/bin/run-mocha.js
Outdated
| @@ -37,6 +37,12 @@ function run(argv, dryRun) { | |||
| ); | |||
| mochaOpts.unshift('--require', sourceMapRegisterPath); | |||
| } | |||
|
|
|||
| // Fail any tests that are printing to console. | |||
| mochaOpts.unshift( | |||
There was a problem hiding this comment.
I suggest that we add --allow-console-logs option (so that @loopback/build can be used by other modules outside of LoopBack 4).
--allow-console-logsis default tofalseif not present--allow-console-logscan be used to override our check
There was a problem hiding this comment.
To be honest, I think this is YAGNI. IMO, every developer out there should have a test suite that's not printing random stuff to console output. If somebody comes to us complaining about this particular feature of lb-mocha, only then it is the right time to implement the flag. It will also allow us to learn that somebody is actually using @loopback/build.
There was a problem hiding this comment.
Our CLI allows generated loopback apps to use @loopback/build.
Since it's trivial to add this option to be consistent with other wrappers, I would moderately prefer to just add it now.
| @@ -6,20 +6,18 @@ | |||
| import {Provider} from '@loopback/context'; | |||
| import {ServerRequest} from 'http'; | |||
| import {LogError} from '../internal-types'; | |||
| import * as debugModule from 'debug'; | |||
| const debug = debugModule('loopback:rest:error'); | |||
|
|
|||
| export class LogErrorProvider implements Provider<LogError> { | |||
There was a problem hiding this comment.
+1 to have a provider.
There was a problem hiding this comment.
Can we add it as an optional dependency of the DefaultSequence?
There was a problem hiding this comment.
It is already a required dependency of the DefaultSequence. Changing it to an optional dependency is out of scope of this pull request IMO. Please note that I am not changing the design of LogError sequence action in this patch, just the implementation details.
There was a problem hiding this comment.
I don't see it being injected at https://github.com/strongloop/loopback-next/blob/e7c6f888a2f9603c7c0ae5dd8c6a4af274e05030/packages/rest/src/sequence.ts#L61. Can you clarify?
There was a problem hiding this comment.
Errors are logged by the reject action. The LogError function is injected into our default implementation of reject, see
This allows us to keep the sequence as a high-level description of steps to perform, and let individual sequence actions to deal with lower-level concerns like error handling and logging.
Modify the command "lb-mocha" to load a helper file that detects calls of console.log/console.error/console.warn APIs and fails the test suite in such case. This forces developers to write tests in such way that the console output remain clean, uncluttered by random logs.
Revert back the recent change that moved error logs from console to debug logs. With this commit in place, when a request handler fails with a 5xx error, this error is printed via `console.error()` to notify people operating the application about a possible problem.
|
@raymondfeng I added support for LGTY now? |
Modify the command "lb-mocha" to load a helper file that detects calls of console.log/console.error/console.warn APIs and fails the test suite in such case. This forces developers to write tests in such way that the console output remain clean, uncluttered by random logs.
This is a follow-up and a partial revert of #939.
The first commit adds a check to verify that no console logs are produced by our tests, beyond what Mocha prints. This is needed to ensure clean test output in the future.
The second commit reverts the default implementation of
LogErrorsequence action to useconsole.erroragain (instead ofdebug) and fixes tests to suppress error messages for requests that fail with the expected error.Checklist
npm testpasses on your machinepackages/cliwere updatedpackages/example-*were updated