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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cli): add responses for PingController.ping() #1755

Merged
merged 1 commit into from
Sep 25, 2018

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Sep 24, 2018

Fixes #1754

Checklist

  • 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

@raymondfeng raymondfeng requested a review from bajtos as a code owner September 24, 2018 22:46
@@ -1,14 +1,41 @@
import {Request, RestBindings, get} from '@loopback/rest';
import {inject} from '@loopback/context';

const PING_RESPONSE = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an explicit type SchemaObject to let the compiler verify correctness of the schema here, where we are defining PING_RESPONSE, instead of deferring the check until PING_RESPONSE is assigned to responses in @get below.

const PING_RESPONSE: SchemaObject = {
  // ...
};

expect(res.body).to.containEql({greeting: 'Hello from LoopBack'});
});

it('exposes OpenAPI spec at /openapi.json', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test name feels misleading to me. We are not testing the entire OpenAPI spec, only the spec of GET /ping endpoint here. How about:

it('provides a valid OpenAPI spec for "GET /ping"', async () => {
 // ...
});

Personally, I don't think we have to make an HTTP request to test that, I would move this test to src/test/integration and obtain the spec via app.restServer.getApiSpec(). Such test will be much more resilient against possible changes of /openapi.json location in the future.

I see the value in a test obtaining the spec via HTTP. I think such test should go to test/acceptance/application.acceptance.ts and verify that the returned spec is valid.

import {validateApiSpec} from '@loopback/testlab';

describe('TodoApplication' /* use the real app class name */, () => {
  it('exposes valid OpenAPI spec at /openapi.json', async () => {
    const res = await client.get('/openapi.json').expect(200);
    await validateApiSpec(res.body);
  });
});

I am ok with leaving such test out of scope of this pull request if you prefer.

@raymondfeng
Copy link
Contributor Author

@bajtos I added typing for PING_RESPONSE and removed the /openapi.json test from the PR. PTAL.

@@ -1,5 +1,32 @@
import {Request, RestBindings, get} from '@loopback/rest';
import {inject} from '@loopback/context';
import {ResponseObject} from '@loopback/openapi-v3-types';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we import ResponseObject from @loopback/rest directly? I believe our rest package is re-exporting OAS types for ease of use.

We are going to remove @loopback/openapi-v3-types soon (before 4.0 GA), see #301.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was concerned about microsoft/TypeScript#26985. Will double-check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I add the code to re-export @loopback-v3-types from @loopback-rest.

@@ -78,6 +78,7 @@
"@loopback/core": "<%= project.dependencies['@loopback/core'] -%>",
"@loopback/dist-util": "<%= project.dependencies['@loopback/dist-util'] -%>",
"@loopback/openapi-v3": "<%= project.dependencies['@loopback/openapi-v3'] -%>",
"@loopback/openapi-v3-types": "<%= project.dependencies['@loopback/openapi-v3-types'] -%>",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to import OAS types from @loopback/rest, see my comment above.

@@ -72,6 +72,7 @@
<% if (project.projectType === 'application') { -%>
"@loopback/core": "<%= project.dependencies['@loopback/core'] -%>",
"@loopback/openapi-v3": "<%= project.dependencies['@loopback/openapi-v3'] -%>",
"@loopback/openapi-v3-types": "<%= project.dependencies['@loopback/openapi-v3-types'] -%>",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants