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

Ability to detect untested paths #260

Open
mefellows opened this issue Dec 16, 2021 · 4 comments
Open

Ability to detect untested paths #260

mefellows opened this issue Dec 16, 2021 · 4 comments
Labels
enhancement New feature or request

Comments

@mefellows
Copy link

mefellows commented Dec 16, 2021

Are you using OpenAPI 2, 3.0.X, or 3.1.0?

3.0.0

Would this solve a problem or make something easier?

Solves a problem.

What would you like to happen?

Checking that your code is compatible with a schema is helpful, but most people I'd argue are looking to say "I have a level of confidence that my code implements the schema (not just part of it)". At the moment, there is no way to know that all paths in a schema have been tested.

An example to illuminate the problem.

Given this OAS file:

openapi: 3.0.0
info:
  title: Example API
  version: 1.0.0
paths:
  /example:
    get:
      responses:
        200:
          description: Response body should be an object with fields 'stringProperty' and 'integerProperty'
          content:
            application/json:
              schema:
                type: object
                required:
                  - stringProperty
                  - integerProperty
                properties:
                  stringProperty:
                    type: string
                  integerProperty:
                    type: integer
        404:
          description: Not found
          content: {}

and this test:

// Import this plugin
import jestOpenAPI from 'jest-openapi';

// Load an OpenAPI file (YAML or JSON) into this plugin
jestOpenAPI('path/to/openapi.yml');

// Write your test
describe('GET /example/endpoint', () => {
  it('should satisfy OpenAPI spec', async () => {
    // Get an HTTP response from your server (e.g. using axios)
    const res = await axios.get('http://localhost:3000/example/endpoint');

    expect(res.status).toEqual(200);

    // Assert that the HTTP response satisfies the OpenAPI spec
    expect(res).toSatisfyApiSpec();
  });
});

You might have the false sense of security that your code implements the specification, but of course the 404 path is untested.

A method such as expectOASIsSatisfied() (or something much better) that checks all the main paths have been tested at least once would be a great place to start.

It's probably going to be impossible to prove all correct paths have been tested (if you consider polymorphic payloads such as oneOf or anyOf), but it should be fairly easy to check if all of the main paths + response code pairings have at least been tested once.

Describe alternatives you've considered

Tools like Dredd and Optic will give you a level of coverage

Additional context or screenshots

n/a

Are you going to resolve the issue?

I'd be open to a PR if I could get a steer on if this is worth doing and possibly where to start.

@mefellows mefellows added the enhancement New feature or request label Dec 16, 2021
@rwalle61
Copy link
Collaborator

Thanks @mefellows for suggesting this! Sorry I'm quite busy over the Christmas period so don't have time to look at this properly right now 🎄

My initial thoughts are that this would be a useful feature. The first two things I'd like to think about and check are:

  • as you've mentioned, there may be complications if we can't check all paths, and we'd have to be very clear what expectOASIsSatisfied() represents ("all the 'main' paths have been tested"?).
  • do Dredd or Optic have anything like this feature already? That would help decide whether adding this feature to this project (which expands the scope a bit) is worth it.

It'd be great if you're happy to answer those two questions for me 🙂 Otherwise I'll look at this when I can, but it won't be this month unfortunately

@mefellows
Copy link
Author

Thanks @mefellows for suggesting this! Sorry I'm quite busy over the Christmas period so don't have time to look at this properly right now 🎄

I totally understand!

do Dredd or Optic have anything like this feature already? That would help decide whether adding this feature to this project (which expands the scope a bit) is worth it.

Yes, Dredd will test all paths by default if they have defined examples/schemas. It was using Dredd on a particular project that led me to investigate other tools. As it's a functional testing tool that runs outside of your code base, it's actually a little annoying to test certain use cases. For example, I needed to test an HTTP 500 that also had a body, which meant that I needed to find a way to hook into a running service to manipulate state and cause an error - whilst not impossible, even on a small project it was going to require a lot of scaffolding/mess.

A unit testing tool like this is much nicer, because I can stub as appropriate. The downside is the missing feature - how do I know I've tested all of the paths?

Optic has, changed since I last looked at it, so will have to drop this for now (looks like they are commercialising the tool and their site has changed).

as you've mentioned, there may be complications if we can't check all paths, and we'd have to be very clear what expectOASIsSatisfied() represents ("all the 'main' paths have been tested"?).

I think the default behaviour should be to ensure all of the paths->operations->responses are covered. That would likely cover the > 90% of use cases. Where it gets messy are cases where a response has a polymorphic schema attached. Detecting that all of those are tested might also be possible, but perhaps that's something that can come later and be configurable on the assertion.

This behaviour could also potentially be configured (e.g. using vendor extensions ) albeit I haven't thought too much on that.

@rwalle61
Copy link
Collaborator

rwalle61 commented Jan 3, 2022

Thanks for the clear comparison with Dredd and suggestion on the default behaviour 🙂 an assertion like expectOASIsSatisfied() (or expect(openApiSpec).toBeSatisfied()) certainly sounds useful.

I'm just wondering if it belongs in this project or a separate one. Here are my thoughts, and I'd like to know what you think:

Currently this project has two assertions:

expect(res).toSatisfyApiSpec();
expect(object).toSatisfySchemaInApiSpec(schemaName);

(I think people use toSatisfyApiSpec but not toSatisfySchemaInApiSpec, since most issues raised are about the former).

Both these assertions run really quickly, and assert against one schema - for a response or an object.

Would expectOASIsSatisfied() read the spec, make requests to your server for all paths in the spec, and validate the responses against the spec? If so, this assertion runs a bit slower than the existing assertions, and asserts against many response schemas (or one OpenAPI spec depending on how you look at it). It does a lot more work and has grey areas (e.g. we define which cases must be satisfied for the whole spec to count as satisfied. Though we could let the user customise this)

(Or, would you still manually call expect(res).toSatisfyApiSpec() for all paths, then at the end call expectOASIsSatisfied() to calculate coverage? (This sounds complicated - I prefer the first option)).

That said, those differences are not huge, and this third assertion is probably still within the project scope:

Problem 😕

If your server's behaviour doesn't match your API documentation, then you need to correct your server, your documentation, or both. The sooner you know the better.

Solution 😄

These test plugins let you automatically test whether your server's behaviour and documentation match. They extend Jest and Chai to support the OpenAPI standard for documenting REST APIs. In your JavaScript tests, you can simply assert expect(responseObject).toSatisfyApiSpec()

The maintenance cost may be significant. Perhaps before working on it we should leave this issue open and see if others upvote it or comment. Or we should spike it and see how useful it is.

What do you think?

@mefellows
Copy link
Author

Sorry for my delay in response here, i've been on leave.

(Or, would you still manually call expect(res).toSatisfyApiSpec() for all paths, then at the end call expectOASIsSatisfied() to calculate coverage? (This sounds complicated - I prefer the first option)).

Actually something like this was more what I was after. The problem with the other option, is that you then need to build in all sorts of hooks etc. to give the caller options to setup state before/after any request (e.g. to mock/stub other components to elicit different responses). At this point, you'll be building something like Dredd.

The attractiveness of this library is that you still get the benefits and creature comforts of a unit testing framework, and if you could gain the benefit of "did I miss anything?" then that's really helpful.

For example, imagine a situation where a completely new endpoint is added to an API and the schema is updated. In this case, without modifying tests using this library, you might be inclined to assume (via a green build) that the API does indeed satisfy the spec, but actually, it just means the paths you tested are compatible and there are obvious gaps in your testing.

Of course, you would have a better view of the complications involved in this option :)

The maintenance cost may be significant. Perhaps before working on it we should leave this issue open and see if others upvote it or comment. Or we should spike it and see how useful it is.

A valid concern indeed. Keen to see if anybody else sees this as something useful / a problem. Happy to spike it out if you had any pointers though.

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

No branches or pull requests

2 participants