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: generate mocks for all responses #1284

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

huwshimi
Copy link
Contributor

Status

READY

Description

Generate mocks for all responses defined in an openapi spec.

Fixes: #1280.

Todos

  • Tests
  • Documentation
  • Changelog Entry (unreleased)

Steps to Test or Reproduce

  1. cd to samples/react-query/status-codes
  2. Run yarn generate-api.
  3. Open samples/react-query/status-codes/src/api/endpoints/petstoreFromFileSpecWithTransformer.msw.ts
  4. See that mocks and http handlers are generated for each response.
  5. At the bottom see that only getListPetsMockHandler() is included in the array of handlers (this is so that when passing the handlers to the worker there is only one http response for each path).

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 wasn't sure if I should commit this sample with multiple responses defined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well I think adding samples/tests is always a good idea. Just my two cents.

Copy link
Member

Choose a reason for hiding this comment

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

I think a sample project is unnecessary. As I commented elsewhere, I want to control this response as an option, and by default I think it is sufficient to output only the minimum success status, so it is sufficient to check it in the test rather than in the sample project.

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've removed the sample project and added a config test.

@melloware melloware added the mock Related to mock generation label Mar 27, 2024
melloware
melloware previously approved these changes Mar 27, 2024
@soartec-lab
Copy link
Member

I'll check it in order 👍

@soartec-lab
Copy link
Member

@huwshimi
Hi, this PR is nice idea.
I thought it was overkill if we only want to get a simple mock.
For example, is it okay to output all statuses only when true is specified with an option like generateEachHttpStatus: boolean ?

@soartec-lab soartec-lab self-assigned this Apr 3, 2024
@huwshimi
Copy link
Contributor Author

huwshimi commented Apr 4, 2024

@huwshimi Hi, this PR is nice idea. I thought it was overkill if we only want to get a simple mock. For example, is it okay to output all statuses only when true is specified with an option like generateEachHttpStatus: boolean ?

Making it optional sounds like a good idea. I've modified this to add that config option.

melloware
melloware previously approved these changes Apr 4, 2024
Copy link
Member

@soartec-lab soartec-lab left a comment

Choose a reason for hiding this comment

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

Thaks! I made a comment, please check it.


if (
generatorOptions.mock &&
'generateEachHttpStatus' in generatorOptions.mock &&
Copy link
Member

Choose a reason for hiding this comment

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

Is this existence check unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.mock can be an object or a function, this is probably more readable if it uses isObject(generatorOptions.mock) so I'll change it to that.

const handlerName = `get${pascal(operationId)}MockHandler`;
const getResponseMockFunctionName = `get${pascal(operationId)}ResponseMock`;

const baseDefinition = generateDefinition(
Copy link
Member

Choose a reason for hiding this comment

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

This is a refactoring consultation.
Is it possible to increase the generated http status with options and then loop the generation targets as shown below?

const isGenerateEachHttpStatus = generatorOptions.mock && 'generateEachHttpStatus' in generatorOptions.mock && generatorOptions.mock.generateEachHttpStatus;
generateStatuses = isGenerateEachHttpStatus ? [...response.types.success, ...response.types.errors] : [response.types.success]

generateStatuses.forEach(
  (statusResponse) => {}
)

With the current implementation, duplicate handlers and mock definition for success will be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a difference between the "base" and other success responses as I didn't want to introduce a breaking change. The "base" response behaves like the response generated before my change, which picks a random success response each time e.g.

export const getListPetsResponseMock = (
  overrideResponse: any = {},
): Okay | Accepted | Success =>
  faker.helpers.arrayElement([ // pick a random "success" response
    Array.from( // the "Okay" type (an array of pets)
      { length: faker.number.int({ min: 1, max: 10 }) },
      (_, i) => i + 1,
    ).map(() =>
      faker.helpers.arrayElement([
        {
          breed: faker.helpers.arrayElement(['Labradoodle'] as const),
         ...
        },
        ...
      ]),
    ),
    { // The "Accepted" response
      code: faker.number.int({ min: undefined, max: undefined }),
      message: faker.word.sample(),
      ...overrideResponse,
    },
    { // The "Success" response
      code: faker.number.int({ min: undefined, max: undefined }),
      message: faker.word.sample(),
      ...overrideResponse,
    },
  ]);

Whereas the other mocks that are generated only return the specific response for a status:

export const getListPetsResponseMock200 = (overrideResponse: any = {}): Okay =>
  Array.from(
    { length: faker.number.int({ min: 1, max: 10 }) },
    (_, i) => i + 1,
  ).map(() =>
    faker.helpers.arrayElement([
      {
        breed: faker.helpers.arrayElement(['Labradoodle'] as const),
        ...
      },
      ...
    ]),
  );

export const getListPetsResponseMock202 = (
  overrideResponse: any = {},
): Accepted => ({
  code: faker.number.int({ min: undefined, max: undefined }),
  message: faker.word.sample(),
  ...overrideResponse,
});

export const getListPetsResponseMock2xx = (
  overrideResponse: any = {},
): Success => ({
  code: faker.number.int({ min: undefined, max: undefined }),
  message: faker.word.sample(),
  ...overrideResponse,
});

I could change it to return a single success response for the "base" response if you would be happy with a breaking change, though it could be difficult to know which response to return (I guess it could be the lowest 2xx number defined in the responses).

Copy link
Member

Choose a reason for hiding this comment

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

@huwshimi

that's sounds good. I had missed this.
In other words, does this mean that when multiple success responses are defined, the mocked types are different, so it is not a duplicate, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@huwshimi

that's sounds good. I had missed this. In other words, does this mean that when multiple success responses are defined, the mocked types are different, so it is not a duplicate, right?

That's correct.

Copy link
Member

@soartec-lab soartec-lab left a comment

Choose a reason for hiding this comment

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

@huwshimi
Thank you for great job !

@soartec-lab soartec-lab merged commit 5cab47d into orval-labs:master Apr 9, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mock Related to mock generation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only 2xx mock responses are generated
3 participants