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

test(remix): Add a boilerplate for Remix SDK integration tests. #5453

Merged
merged 19 commits into from
Jul 27, 2022

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Jul 22, 2022

Adds an integration testing boilerplate for Remix SDK and sample tests for client and server.

As the line between client and server is blurred in Remix sources, all scenarios are defined in a common Remix application, but tested separately for client and server.

Server

Using the utilities we have already implemented for node-integration-tests. Needs some extra features though, such as triggering post requests to test action functions.

Question 1: Seems to be working well so far, but Remix uses fetch internally by polyfilling it, and nock is http-only so might need to think about an alternative for nock for newer node versions in the future?

Client

Also used the helper utilities from our Playwright browser tests, did not need to implement fixtures or anything, as it seems these tests will be fairly simple.

Question 2: Assuming we can also use those utilities from browser/node integration tests for other SDKs, it might be nice to convert those to an internal package like @sentry-internal/integration-test-utils. This set of tests uses them by re-exporting via relative paths, and that doesn't look great IMO. WDYT?

@onurtemizkan onurtemizkan force-pushed the onur/remix-integration-tests branch from fd80fe1 to bcd82bd Compare July 22, 2022 15:51
@onurtemizkan onurtemizkan force-pushed the onur/remix-integration-tests branch 2 times, most recently from 71babdd to 02a8e82 Compare July 25, 2022 14:24
@onurtemizkan onurtemizkan force-pushed the onur/remix-integration-tests branch from 02a8e82 to 49fc12b Compare July 25, 2022 16:22
@onurtemizkan onurtemizkan marked this pull request as ready for review July 26, 2022 12:08
@AbhiPrasad AbhiPrasad mentioned this pull request Jul 26, 2022
26 tasks
@AbhiPrasad
Copy link
Member

Question 1: Seems to be working well so far, but Remix uses fetch internally by polyfilling it, and nock is http-only so might need to think about an alternative for nock for newer node versions in the future?

Until it breaks, lets stick with this.

Question 2: Assuming we can also use those utilities from browser/node integration tests for other SDKs, it might be nice to convert those to an internal package like @sentry-internal/integration-test-utils. This set of tests uses them by re-exporting via relative paths, and that doesn't look great IMO. WDYT?

Yes let's move stuff into a utils package. In fact, let's make it @sentry-internal/test-utils, so we can put unit test utils in there as well.

@AbhiPrasad
Copy link
Member

Actually it seems @lobsterkatie worked on #5038 - which introduces a dev-utils package. Can we merge that in and use that?

@AbhiPrasad AbhiPrasad self-requested a review July 26, 2022 17:50
@lobsterkatie
Copy link
Member

Actually it seems @lobsterkatie worked on #5038 - which introduces a dev-utils package. Can we merge that in and use that?

Ah, yeah, that got hung up on... something. I'll dig it back up and maybe it will come to me. Anyway, yeah, let's at least get the skeleton in there, and then we can move stuff into it over time. Eventually I'd love to see the node and browser integration tests move into their respective package directories, and consolidating the utility functions seems like a good start.

I'll look at it again tomorrow or early next week.

@AbhiPrasad
Copy link
Member

Alright so let's stick with this for now then, and move stuff over to a package once Katie is done with her work (feel free to pass it off btw - I'm sure any one of us can take it to the finish line).

@onurtemizkan will do a more detailed review in a bit.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

This is a great start!

@AbhiPrasad AbhiPrasad merged commit d8313f3 into master Jul 27, 2022
@AbhiPrasad AbhiPrasad deleted the onur/remix-integration-tests branch July 27, 2022 17:41
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