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

[no-Jira] Make enum mocking deterministic #1155

Merged
merged 2 commits into from
Oct 31, 2024
Merged

[no-Jira] Make enum mocking deterministic #1155

merged 2 commits into from
Oct 31, 2024

Conversation

canac
Copy link
Contributor

@canac canac commented Oct 23, 2024

Description

<GqlMockedProvider> was picking a random value for enums that could vary between tests. That was a source of flaky tests. This PR switches to using our new CruGlobal fork, which contains the stable random enum fix in CruGlobal/graphql-ergonomock#1.

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@canac canac self-assigned this Oct 23, 2024
Copy link
Contributor

github-actions bot commented Oct 23, 2024

Bundle sizes [mpdx-react]

Compared against 121cd06

No significant changes found

@canac canac requested a review from dr-bizz October 23, 2024 20:10
Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

I see that you've altered the yarn.lock file and files inside .yarn. Was there no way to alter the package.json file to do these changes?

@canac
Copy link
Contributor Author

canac commented Oct 24, 2024

graphql-ergonomock is an unmaintained project (no commits in 3 years and no recent responses on bug reports), or I'd submit an upstream PR. That's why I'm making the fix to that package as a yarn patch. I can vendor the package and copy all the files into our project so that we can make changes freely if you'd prefer. I think that publishing a fork to npm is probably more effort than it's worth.

@canac
Copy link
Contributor Author

canac commented Oct 28, 2024

@dr-bizz Do you have any more thoughts on this?

@wrandall22 Maybe you have input too? For perspective, there are two patches we've made to graphql-ergonomock. One to support graphql v15, and this one to fix a bug with deterministic mocking. I feel like that's acceptable for a patch and if we have to change anything else in the future, maybe we consider vendoring the package.

@wrandall22
Copy link
Contributor

That is probably fine. I agree that if we keep making changes we should fork the repo.

@dr-bizz
Copy link
Contributor

dr-bizz commented Oct 28, 2024

@canac
I recommend that we fork the repo and make the changes there. This is how I've done it in the past, and it better prepares future developers to make changes if needed.

Also, what if the original package is updated in the future? If you use a fork, you can merge those updates into your forked version. This helps keep your version in sync with any upstream improvements.

@canac
Copy link
Contributor Author

canac commented Oct 28, 2024

I recommend that we fork the repo and make the changes there.

@dr-bizz One challenge in this case is that our application doesn't consume the raw source code from the graphql-ergonomock package, we consume the published build files. So it's not as simple as forking the repo and making the package a git dependency in package.json. Correct me if I'm mistaken, but we'll have to publish our fork to npm so that we can publish the built files directly. Unless you know of away to get NextJS to run the TS/TSX transforms on package code. Setting up an npm deployment pipeline seems like more effort than it's worth. We could however push the built dist/ files to our fork and then it could be a simple git dependency, if you like that idea.

This complexity is why I suggested vendoring the code since it is a small package (11 files including test files). That will let us run our JSX and TypeScript transforms on the package code. We won't get future updates without manually pulling them in. But the package shows all signs of being abandoned, so I'm not convinced it's worth doing extra work now just in case the upstream repo becomes active again.

@dr-bizz
Copy link
Contributor

dr-bizz commented Oct 28, 2024

Yes, you will need to create an NPM package for this forked repo. This is how I've done it in the past. onesky-utils is a good example. I created Cru's own version of the code when the package wasn't updated and I needed to add a fix. They have since updated their app, so I could migrate God Tools back onto their version and close this one.

@dr-bizz
Copy link
Contributor

dr-bizz commented Oct 28, 2024

I have invited you to CruGlobal NPM organisation

@canac canac force-pushed the stable-random-enum branch from b170320 to 887daa4 Compare October 31, 2024 17:36
@canac canac force-pushed the stable-random-enum branch from 887daa4 to 8c19c14 Compare October 31, 2024 17:37
@canac canac requested a review from dr-bizz October 31, 2024 17:52
@@ -144,7 +144,7 @@
"eslint-plugin-react": "^7.31.8",
"full-icu": "^1.5.0",
"glob": "^8.0.3",
"graphql-ergonomock": "^1.2.0",
"graphql-ergonomock": "npm:@cruglobal/graphql-ergonomock@^1.2.1",
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 figured out that we can use this syntax to use our fork without having to update all of our imports.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know this was a feature. This is a great way to add it!

Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

Great work! Thank you for showing me "npm:{package}@{version}"

@@ -144,7 +144,7 @@
"eslint-plugin-react": "^7.31.8",
"full-icu": "^1.5.0",
"glob": "^8.0.3",
"graphql-ergonomock": "^1.2.0",
"graphql-ergonomock": "npm:@cruglobal/graphql-ergonomock@^1.2.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know this was a feature. This is a great way to add it!

@canac canac merged commit 9d15d74 into main Oct 31, 2024
18 checks passed
@canac canac deleted the stable-random-enum branch October 31, 2024 18:34
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