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

Use Mock Service Worker #427

Merged
merged 11 commits into from Oct 19, 2022
Merged

Use Mock Service Worker #427

merged 11 commits into from Oct 19, 2022

Conversation

oo-bldrs
Copy link
Contributor

@oo-bldrs oo-bldrs commented Oct 7, 2022

Mock Service Worker is a tool for mocking APIs by operating as a service worker and intercepting network requests.

This PR replaces the existing Octokit mock (and its associated conditionals) with the native Octokit library and interception of specific URLs to achieve the same mocking functionality as before.

MSW will allow us to mock other network calls in the future as we expand the functionality of Share.

No changes to unit tests were necessary and all continue to pass as before.

@netlify
Copy link

netlify bot commented Oct 7, 2022

Deploy Preview for bldrs-share ready!

Name Link
🔨 Latest commit 752dd72
🔍 Latest deploy log https://app.netlify.com/sites/bldrs-share/deploys/634d6df9378e010009d85027
😎 Deploy Preview https://deploy-preview-427--bldrs-share.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@oo-bldrs
Copy link
Contributor Author

oo-bldrs commented Oct 7, 2022

This works when I test it locally on my machines (both primary and secondary with a clean directory) but not in GitHub actions. I'll need to take a look at this later.

Test Suites: 27 passed, 27 total
Tests:       93 passed, 93 total
Snapshots:   0 total
Time:        4.632 s
Ran all test suites.
✨  Done in 5.30s.

@pablo-mayrgundter
Copy link
Member

Did you see #421? We introduced https://github.com/bldrs-ai/Share/blob/main/__mocks__/%40octokit/rest.js

That achieves a similar goal and gives us more explicit control of preconditions for tests.

One reason for having the MockOctokit was to enable localhost serving during dev. I often hit GH rate limit for comments without it. Now realizing the name is confusing. LocalOctokit?

@oo-bldrs
Copy link
Contributor Author

oo-bldrs commented Oct 7, 2022

@pablo-mayrgundter I did not see #421 prior to this PR, thank you for pointing that out.

I'm not sure I understand your statement about controlling test preconditions, do you mean being able to set expected request paths per individual test? If not, can you elaborate?

https://www.danieljcafonso.com/configuring_msw

MSW intercepts requests made in the browser, meaning if you define a route handler (and corresponding response), it will be handled locally and not touch the Internet despite the target URL having an external URL -- GitHub rate limits won't come into play.

https://github.com/bldrs-ai/Share/pull/427/files#diff-a4994793da5ecfe4df45523fd07d4c2d368d46c4d3c2a3e4d902d11175024ab7R9

MSW's request interception allows us to have a single code path regardless of whether we're running in a local development environment, a CI/CD pipeline, or in production.

As part of the testing for the authentication features, I need to be able to assert against headers (or their lack thereof) and send back line level responses (e.g. 401 Unauthorized) that are processed natively by Octokit.

Function-level mocking of Octokit doesn't get me there and it would get fairly messy (and fragile) to inject a HTTP client instance into Octokit to achieve the same level of functionality, if that's even possible.

Copy link
Member

@pablo-mayrgundter pablo-mayrgundter left a comment

Choose a reason for hiding this comment

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

Per convo, please merge in #421 where we mocked out Octokit to make sure this still services that test case.

src/index.jsx Show resolved Hide resolved
We only care if an unexpected call is made to api.github.com, as opposed
to the default of raising a console warning about everything.

The presence of a MSW console error/warning means that we are trying to
exercise GitHub API functionality that we haven't yet mocked.

Currently, we're raising a console error but as our confidence
increases, we can change this to throw an exception that will halt
execution.
@oo-bldrs
Copy link
Contributor Author

@pablo-mayrgundter PTAL, I've merged in the code from #421 and added logic to the build process to delete the MSW static asset from non-development builds.

@oo-bldrs oo-bldrs merged commit a03ba80 into bldrs-ai:main Oct 19, 2022
@oo-bldrs oo-bldrs deleted the use-msw branch October 19, 2022 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants