refactor: move unexpectedHttpErrorLogger to testlab

[nabdelgadir]

See #2803 (comment) for motivation.

Moving the unexpectedHttpErrorLogger from @loopback/rest to @loopback/testlab so it can be used for tests in other packages/examples.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

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

👉 Check out how to submit a PR 👈

[raymondfeng]

Should we use 200 as the default value?

[raymondfeng]

Can we accept ...expectedStatusCodes: number[] to allow multiple status codes without breaking existing code?

[bajtos]

The purpose of this helper is to disable error logs printed by our default error handler for the status code expected by an integration/end-to-end test making HTTP calls to the tested application.

Typically, such test is verifying the response status code against a single value only.

await client.get('/foo/bar').expect(404);

Should we use 200 as the default value?

That does not make much sense to me - no error is logged for 2xx and 3xx status codes. This helper is useful only for tests expecting 4xx or 5xx response.

Can we accept ...expectedStatusCodes: number[] to allow multiple status codes without breaking existing code?

I think so. However, I am not convinced this a good practice to support. Let's defer this enhancement until we have a real-world use case needing it.

[raymondfeng]

That does not make much sense to me - no error is logged for 2xx and 3xx status codes. This helper is useful only for tests expecting 4xx or 5xx response.

Then why don't we have the following signature? It leaves the default to undefined instead of 0 to avoid similar confusions as I had.

export function createUnexpectedHttpErrorLogger(
  expectedStatusCode? number,
) {}

[bajtos]

I don't remember why I picked 0 as the default value. Your proposal to use undefined instead of 0 is fine with me 👍

[nabdelgadir]

Updated to use undefined. 👍

[bajtos]

Code changes LGTM.

Let's document the new helper please.

[raymondfeng]

Please also check the test coverage - https://coveralls.io/builds/23172908/source?filename=packages%2Ftestlab%2Fsrc%2Fhttp-error-logger.ts#L14

[bajtos]

Please also check the test coverage - https://coveralls.io/builds/23172908/source?filename=packages%2Ftestlab%2Fsrc%2Fhttp-error-logger.ts#L14

Good catch!

I think we should disable code coverage for the console.error branch, because this branch is never executed on successful CI builds.

I think /* istanbul ignore next */ is the tool to use? See https://github.com/gotwarlost/istanbul/blob/master/ignoring-code-for-coverage.md

[nabdelgadir]

The test I created initially does pass, but it requires @loopback/rest which causes a circular dependency, so I had to remove it.

Since I'm squashing the commits, here's the test mentioned:

import {HttpErrors, RestApplication, SequenceActions} from '@loopback/rest';
import * as sinon from 'sinon';
import {createUnexpectedHttpErrorLogger} from '../..';
import {createRestAppClient} from '../../client';
import {expect} from '../../expect';

describe('createUnexpectedHttpErrorLogger', () => {
  it('does not log an error if the status code is expected', async () => {
    const errorLogger = createUnexpectedHttpErrorLogger(401);
    const app = new RestApplication();
    const spy = sinon.spy(console, 'error');

    function throwUnauthorizedError() {
      throw new HttpErrors.Unauthorized('Unauthorized!');
    }
    app.route('get', '/', {responses: {}}, throwUnauthorizedError);

    app.bind(SequenceActions.LOG_ERROR).to(errorLogger);

    await app.start();
    const client = createRestAppClient(app);

    await client.get('/');
    expect(spy.notCalled).to.be.true();
    spy.restore();
  });
});

[bajtos]

LGTM as far as I am concerned, but see my suggestion below on how to improve the docs.

Please consider addressing Raymond's comment above and getting his approval too.

[bajtos]

This helps surpress the console from printing an expected error.

Let's improve this part. My proposal - feel free to change it as you like:

This is useful when writing tests for error responses:

- We don't want any error message printed to the console when 
  the server responds with the expected error and the test passes.

- When something else goes wrong and the server returns an unexpected 
  error status code, we do want an error message to be printed to the console 
  so that we have enough information to troubleshoot the failing test.