Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add stub service section #2879

Merged
merged 2 commits into from May 15, 2019

Conversation

Projects
None yet
6 participants
@b-admike
Copy link
Member

commented May 13, 2019

Fixes #2101

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 馃憟

@b-admike b-admike requested review from bajtos and raymondfeng as code owners May 13, 2019

@b-admike b-admike self-assigned this May 13, 2019

geocodeStub.resolves([{y: 0, x: 0}]);
```

Verify how the stub was executed:

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 13, 2019

Contributor

nitpick: // before the sentence.

This comment has been minimized.

Copy link
@b-admike

b-admike May 13, 2019

Author Member

This is explaining the step that is about to happen like Configure the geocode method as a stub and its behaviour before your test(s): above, so it's not meant to be a comment. WDYT @jannyHou?

```ts
// expect that geoService.geocode() was called with the first
// argument equal to the provided address string
sinon.assert.calledWithMatch(geocodeStub, '1600 Pennsylvania Ave NW');

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 13, 2019

Contributor

Hmm, where is '1600 Pennsylvania Ave NW' used in the previous code?

This comment has been minimized.

Copy link
@b-admike

b-admike May 13, 2019

Author Member

Good question. It's not apparent here, but the way we structure our test is to arrange, act, then assert, so somewhere in the act phase, we'd call a controller method that eventually calls the geoService.geocode() with the given address string. I don't show that here because I thought it would be apparent, but I can add a note for that to make it clear.

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 14, 2019

Contributor

I see, thank you for the explanation and good document 馃憤

@nabdelgadir

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

I tried it with the following setup:

import {GeocoderService} from './geocoder.service';
const GeoPoint = require('geopoint');

export class DummyGeocoder implements GeocoderService {
  // in practice, the constructor could take an API key to authenticate with
  // the service proxy provider
  constructor() {}

  geocode(address: string) {
    // this would call the remote API for the real coordinates
    return new GeoPoint(address);
  }
}

Test:

import {DummyGeocoder} from '../../../services/dummy.service';
import {sinon} from '@loopback/testlab';

describe('GeoCoderController', () => {
  let geoService: DummyGeocoder;
  beforeEach(givenStubbedService);

  // your unit tests

  it('', () => {
    const geocodeStub = geoService.geocode as sinon.SinonStub;
    geocodeStub.resolves([{y: 0, x: 0}]);

    sinon.assert.calledWithMatch(geocodeStub, '1600 Pennsylvania Ave NW');
  });

  function givenStubbedService() {
    geoService = sinon.createStubInstance(DummyGeocoder);
  }
});

But it fails with AssertError: expected geocode to be called with match. geocode never gets called, so is there something wrong with the setup?

@b-admike

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

I tried it with the following setup:

import {GeocoderService} from './geocoder.service';
const GeoPoint = require('geopoint');

export class DummyGeocoder implements GeocoderService {
  // in practice, the constructor could take an API key to authenticate with
  // the service proxy provider
  constructor() {}

  geocode(address: string) {
    // this would call the remote API for the real coordinates
    return new GeoPoint(address);
  }
}

Test:

import {DummyGeocoder} from '../../../services/dummy.service';
import {sinon} from '@loopback/testlab';

describe('GeoCoderController', () => {
  let geoService: DummyGeocoder;
  beforeEach(givenStubbedService);

  // your unit tests

  it('', () => {
    const geocodeStub = geoService.geocode as sinon.SinonStub;
    geocodeStub.resolves([{y: 0, x: 0}]);

    sinon.assert.calledWithMatch(geocodeStub, '1600 Pennsylvania Ave NW');
  });

  function givenStubbedService() {
    geoService = sinon.createStubInstance(DummyGeocoder);
  }
});

But it fails with AssertError: expected geocode to be called with match. geocode never gets called, so is there something wrong with the setup?

Thanks for trying it out @nabdelgadir. This is missing the act stage of the test between the arrange/ setup of the test and the assertion for the expected result. A good example to that would be https://github.com/strongloop/loopback-next/blob/master/examples/todo/src/__tests__/unit/controllers/todo.controller.unit.ts#L66. That would invoke the geocode method with the address string and then the assertion should work. I'll add an assertion in that test and refer to it in the docs.

```

Check out
[this](<(https://github.com/strongloop/loopback-next/blob/master/examples/todo/src/__tests__/unit/controllers/todo.controller.unit.ts#L53-L71)>)

This comment has been minimized.

Copy link
@dhmlau

dhmlau May 13, 2019

Contributor

From the github view, the this doesn't show the link. do you mean:

[this](https://github.com/strongloop/loopback-next/blob/master/examples/todo/src/__tests__/unit/controllers/todo.controller.unit.ts#L53-L71)

Also, I'm not sure specifying the line number would be a good solution in general, because they tend to get outdated easily. But I'm not sure if specifying the test case name would be a good alternative either. Just wanna bring this up. Thanks.

This comment has been minimized.

Copy link
@bajtos

bajtos May 14, 2019

Member
  1. Let's use a more descriptive link name than "this". For example, "TodoController unit tests".

  2. Either point to the entire file in master branch (https://github.com/strongloop/loopback-next/blob/master/examples/todo/src/__tests__/unit/controllers/todo.controller.unit.ts) or lock down the commit sha in a link pointing to line numbers (https://github.com/strongloop/loopback-next/blob/bd0c45033503f631a533ad6176620354d9cf6768/examples/todo/src/__tests__/unit/controllers/todo.controller.unit.ts#L53-L71).

This comment has been minimized.

Copy link
@b-admike

b-admike May 14, 2019

Author Member

Yes I was looking for the commit sha, thank you both for pointing this out :-).

@bajtos
Copy link
Member

left a comment

The changes look good at high level, let's discuss few details.

Show resolved Hide resolved docs/site/Testing-your-application.md
```

Check out
[this](<(https://github.com/strongloop/loopback-next/blob/master/examples/todo/src/__tests__/unit/controllers/todo.controller.unit.ts#L53-L71)>)

This comment has been minimized.

Copy link
@bajtos

bajtos May 14, 2019

Member
  1. Let's use a more descriptive link name than "this". For example, "TodoController unit tests".

  2. Either point to the entire file in master branch (https://github.com/strongloop/loopback-next/blob/master/examples/todo/src/__tests__/unit/controllers/todo.controller.unit.ts) or lock down the commit sha in a link pointing to line numbers (https://github.com/strongloop/loopback-next/blob/bd0c45033503f631a533ad6176620354d9cf6768/examples/todo/src/__tests__/unit/controllers/todo.controller.unit.ts#L53-L71).

@b-admike b-admike force-pushed the docs/service-stub branch 2 times, most recently from 4facc95 to 3a6e327 May 14, 2019

@b-admike

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

@bajtos @dhmlau @jannyHou @nabdelgadir I think I've applied all your feedback. LGTY now?

@bajtos

bajtos approved these changes May 14, 2019

Copy link
Member

left a comment

馃憦

@nabdelgadir
Copy link
Contributor

left a comment

LGTM besides a nitpick 馃憤

}
```

The first step is to create a mocked instance of the `GeoCoderService` API and

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir May 14, 2019

Contributor

Nitpick: GeoCoderService -> GeocoderService (there's a few other instances below nvm, just one)

@jannyHou
Copy link
Contributor

left a comment

馃挴 LGTM

@dhmlau

dhmlau approved these changes May 14, 2019

Copy link
Contributor

left a comment

I'm not familiar with the stub service, but you've addressed my question/concern.

@b-admike b-admike force-pushed the docs/service-stub branch 3 times, most recently from 2e973ae to e3b68e8 May 14, 2019

b-admike added some commits May 13, 2019

@b-admike b-admike force-pushed the docs/service-stub branch from e3b68e8 to fc965e4 May 15, 2019

@b-admike b-admike merged commit b5ea915 into master May 15, 2019

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 91.731%
Details

@b-admike b-admike deleted the docs/service-stub branch May 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.