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.

Sign up for GitHub

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

Jump to bottom

feat(msw): split polymorphic mocks into its own functions #1365

Merged
merged 11 commits into from
May 19, 2024
Merged

feat(msw): split polymorphic mocks into its own functions #1365

merged 11 commits into from
May 19, 2024

Conversation

AllieJonsson
Copy link
Contributor

@AllieJonsson AllieJonsson commented May 13, 2024 •
edited
Loading

Status

READY

Description

Fixes #1318
This splits response mocks where the response contains objects that have a base object.
The specification in the linked issue is an example of a response mock that will get split.
image

Todos

  • Tests
  • Documentation
  • Changelog Entry (unreleased)

Steps to Test or Reproduce

  1. Generate using a specification that contains a allOf prop, e.g. the one in the linked issue.
  2. Notice that the response mock contains calls to functions that has been generated above.
  3. Notice that no errors exists

@melloware melloware added the mock Related to mock generation label May 13, 2024
@soartec-lab soartec-lab self-assigned this May 13, 2024
@soartec-lab
Copy link
Member

@AffeJonsson

A conflict occurred when I merged the following PR. Could you please check again and correct it?

https://github.com/anymaniax/orval/pulls?q=is%3Apr+is%3Aclosed

@AllieJonsson
Copy link
Contributor Author

AllieJonsson commented May 14, 2024 •
edited
Loading

@AffeJonsson

A conflict occurred when I merged the following PR. Could you please check again and correct it?

https://github.com/anymaniax/orval/pulls?q=is%3Apr+is%3Aclosed

Merged master and did a small update to make overrideResponse param Partial<name>, just like the new overrideResponse code

melloware
melloware previously approved these changes May 14, 2024
View reviewed changes
@melloware melloware added this to the 6.29.0 milestone May 14, 2024
@AllieJonsson
Copy link
Contributor Author

Maybe wait with this until #1199 is complete? I assume we might need to change this when it is completed anyway

@soartec-lab
Copy link
Member

@AffeJonsson

No, they says it will take time, so we unnecessary wait. And. I'll share it to they.
I would like to check this PR, but please forgive me if it takes a little more time.

@karlismelderis-mckinsey
Copy link
Contributor

it would make sense to regenerate samples

example in screenshot has overrideResponse twice 🤔

...overrideResponse
// some faker
...overrideResponse

@AllieJonsson
Copy link
Contributor Author

it would make sense to regenerate samples

example in screenshot has overrideResponse twice 🤔

...overrideResponse
// some faker
...overrideResponse

This is no longer the case after your PR was merged, only one overrideResponse is now present

soartec-lab
soartec-lab reviewed May 15, 2024
View reviewed changes
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.

@AffeJonsson
Thank you for great update. I check it and commented.

packages/mock/src/faker/resolvers/value.ts Outdated
allowOverride,
});
if (newSchema.allOf) {
Copy link
Member

Choose a reason for hiding this comment

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

Not only can objects be separated using oneOf, but also objects can now be separated using allOf? ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake, I did this backwards. I have now changed this to look at the combine.separator === 'oneOf instead, which is much better.

packages/mock/src/types.ts Outdated
@@ -8,6 +8,7 @@ export interface MockDefinition {
name: string;
overrided?: boolean;
includedProperties?: string[];
functions?: string[];
Copy link
Member

Choose a reason for hiding this comment

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

I would like to rename functions for a couple of reasons.

  1. It is difficult to tell whether it is a list of typescript Function or a collection of function definitions as a string.
  2. MockDefinition already has a field value to get the value, so we need to clarify what value this field is.

If it's functions of split mock definition, how about splitMockImplementions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, done!

@AllieJonsson
Copy link
Contributor Author

Seems I broke tests with this, fixing...

soartec-lab
soartec-lab reviewed May 17, 2024
View reviewed changes
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.

@AffeJonsson
I checked again and requested a minor correction. Please check.

packages/mock/src/msw/index.ts Outdated
);
splitMockImplementations.push(...addedSplitMockImplementations);
const mockImplementations = addedSplitMockImplementations.length
? `//#region ${getResponseMockFunctionName} sub functions\n${addedSplitMockImplementations.join('\n\n')}\n//#endregion\n\n`
Copy link
Member

Choose a reason for hiding this comment

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

If you want to include annotate, I think another way to express them using variable names would be appropriate. Or i think you can just delete it if it's not necessary.

packages/mock/src/msw/mocks.ts Outdated
}) => {
// console.log(responses);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like I forgot to turn this off.

@melloware melloware modified the milestones: 6.29.0, 6.30.0 May 17, 2024
@soartec-lab
Copy link
Member

@AffeJonsson
Please let me know when this is ready for me to review 👍

@AllieJonsson
Copy link
Contributor Author

@AffeJonsson Please let me know when this is ready for me to review 👍

It's ready!

soartec-lab
soartec-lab approved these changes May 19, 2024
View reviewed changes
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.

Thanks!

@soartec-lab soartec-lab merged commit ddf7964 into orval-labs:master May 19, 2024
2 checks passed
@AllieJonsson AllieJonsson deleted the feat/split-poly-mocks branch May 20, 2024 04:46
@AllieJonsson AllieJonsson mentioned this pull request Jun 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soartec-lab soartec-lab soartec-lab approved these changes

@melloware melloware melloware approved these changes

@anymaniax anymaniax Awaiting requested review from anymaniax

Assignees

@soartec-lab soartec-lab

Labels
mock Related to mock generation
Projects
None yet
Milestone
6.30.0
Development

Successfully merging this pull request may close these issues.

Feature request: Split polymorphic mocks into it's own functions
4 participants
@AllieJonsson @soartec-lab @karlismelderis-mckinsey @melloware