fix: remove console output from tests #939

raymondfeng · 2018-01-31T00:36:58Z

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Related API Documentation was updated
Affected artifact templates in packages/cli were updated
Affected example projects in packages/example-* were updated

virkt25

LGTM. Windows build needs to pass. Seems like a random AppVeyor failure though.

raymondfeng · 2018-01-31T05:19:44Z

@virkt25 The Windows failure on Node 6 (https://ci.appveyor.com/project/bajtos/loopback-next/build/1.0.3452-master/job/xs23p54blwivxy61) has been happening a lot recently. It seems to be a npm issue but no definite answer is found on Google.

virkt25 · 2018-01-31T05:36:50Z

Yea I've seen AppVeyor failing a lot more on recent PRs.

bajtos

@raymondfeng While I appreciate your effort on cleaning the console output of npm test, I am afraid your changes severely reduced developer experience while troubleshooting loopback-next tests. See my comment below.

In the future, I would really appreciate if pull requests like this one could be left open for longer period of time, allowing me to post my review before the changes are merged ☹️

bajtos · 2018-02-01T10:02:47Z

packages/example-log-extension/src/providers/log-action.provider.ts

          break;
      }
    }
  }
+
+  log(msg: string) {
+    debug(msg);


I think this makes our example-log-extensions sort of useless. The point of the extension is to let applications produce logs that are useful in production. Now we are logging via debug which is almost always turned off in production environments :(

IMO, the log component should:

print to console by default

allow tests to choose a different target (e.g. debug).

Since the problem of cluttered test output is not specific to us only, but will apply to anybody building an application with a logger component, I think it's even more important for our example log component to show an elegant solution for suppressing console logs in tests.

I'll take a slab.

bajtos · 2018-02-01T10:06:38Z

packages/example-log-extension/src/providers/log-level.provider.ts

@@ -10,6 +10,8 @@ export class LogLevelProvider implements Provider<number> {
  constructor() {}

  value(): number {
+    const level = Number(process.env.LOG_LEVEL);


-1000

Internal implementation should not access process.env directly, because it introduces hidden coupling that's difficult to discover (learn about its existence) and make it difficult to customize the behavior in specific places only, without affecting everybody else living in the same Node.js process.

If we want to access process.env.LOG_LEVEL, the such code should live somewhere in the main application file where we are constructing and booting an Application instance.

To illustrate the provider concept for logging level, we need to have a variable. Let me see what else we can use instead of env var.

The provider returns a value that is bound via the component. The user can override the log-level but re-binding the value to a different value explicitly in the app.

bajtos · 2018-02-01T10:07:40Z

packages/example-log-extension/test/acceptance/log.extension.acceptance.ts

  }

-  function restoreConsoleSpy() {
+  function restoreLogSpy() {


This code is duplicated here and in packages/example-log-extension/test/unit/providers/log-action.provider.unit.ts, please move both createandrestore` helpers to a shared file.

bajtos · 2018-02-01T10:12:42Z

packages/rest/src/providers/log-error-provider.ts

      return;
    }

-    console.error(
+    debug(


-1

Printing failed HTTP requests to stderr is intentional behavior that needs to be preserved as the default mode of LoopBack4+ applications.

This is important for our unit-tests too, because when a request fails with an HTTP status code we did not expect, then we do want to see full stack trace of the server error in the test output. For example, in a test checking that a request for unknown path returns 404, we want to see server stack trace when the request failed with 500 Internal Error caused e.g. by TypeError or ReferenceError.

We already have a solution for suppressing these messages you can use for inspiration, see here:

https://github.com/strongloop/loopback-next/blob/ff4a320825720270346013ce81174100bef28f37/packages/rest/test/integration/http-handler.integration.ts#L503-L517

bajtos · 2018-02-01T10:14:57Z

packages/rest/test/integration/http-handler.integration.ts

@@ -521,26 +527,11 @@ describe('HttpHandler', () => {
  function givenClient() {
    client = createClientForHandler((req, res) => {
      handler.handleRequest(req, res).catch(err => {
-        console.error('Request failed.', err.stack);
+        debug('Request failed.', err.stack);


This console log must be preserved, see my comment above. If there is a test that's triggering this code path as part of the expected operation, then please use a mechanism similar to skipStatusCode above to allow tests to control which errors to silently ignore.

This PR is a follow-up to the following issue based on review comments: #939 - Demonstrate how to plug in a different log (console vs memory) - Switch to @loopback/metadata for decorator implementation - Remove log level provider as it's supposed to be contributed by the app The exercise also exposes a few needs: 1. Support for optional dependency so that we can fall back a default implementation (or a better way) 2. Allow a component to bind artifacts beyond providers

fix: remove console output from tests

8529f3f

See #935

raymondfeng requested review from bajtos, kjdelisle and virkt25 as code owners January 31, 2018 00:36

raymondfeng added the needs-priority label Jan 31, 2018

virkt25 approved these changes Jan 31, 2018

View reviewed changes

raymondfeng merged commit ff4a320 into master Jan 31, 2018

raymondfeng removed the needs-priority label Jan 31, 2018

raymondfeng deleted the fix-console-output branch February 1, 2018 00:24

bajtos reviewed Feb 1, 2018

View reviewed changes

raymondfeng mentioned this pull request Feb 1, 2018

fix: clean up example-log-extension #952

Merged

6 tasks

bajtos mentioned this pull request Feb 27, 2018

fix(rest): log unexpected errors to console #1058

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove console output from tests #939

fix: remove console output from tests #939

raymondfeng commented Jan 31, 2018 •

edited

Loading

virkt25 left a comment

raymondfeng commented Jan 31, 2018

virkt25 commented Jan 31, 2018

bajtos left a comment

bajtos Feb 1, 2018

raymondfeng Feb 1, 2018

bajtos Feb 1, 2018

raymondfeng Feb 1, 2018

virkt25 Feb 1, 2018

bajtos Feb 1, 2018

bajtos Feb 1, 2018

bajtos Feb 1, 2018

fix: remove console output from tests #939

fix: remove console output from tests #939

Conversation

raymondfeng commented Jan 31, 2018 • edited Loading

Checklist

virkt25 left a comment

Choose a reason for hiding this comment

raymondfeng commented Jan 31, 2018

virkt25 commented Jan 31, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Jan 31, 2018 •

edited

Loading