Skip to content

Conversation

duyhungtnn
Copy link
Collaborator

@duyhungtnn duyhungtnn commented Nov 18, 2024

The commit 2099af0 addresses a test failure caused by an outdated & no longer maintain cross-fetch library and changes in the mswjs interface.

E2e passed
https://github.com/bucketeer-io/javascript-client-sdk/actions/runs/11887259300

@duyhungtnn duyhungtnn self-assigned this Nov 18, 2024
@duyhungtnn duyhungtnn changed the title fix: test fail after bump deps fix: test fail after update non-major deps Nov 18, 2024
test/utils.ts Outdated
): SetupServer {
const server = setupServer(...handlers)
server.listen({ onUnhandledRequest: 'error' })
server.listen({ onUnhandledRequest: 'warn' })
Copy link
Collaborator Author

@duyhungtnn duyhungtnn Nov 18, 2024

Choose a reason for hiding this comment

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

In the new mswj version, setting onUnhandledRequest: 'error' throws an error for unhandled requests instead of doing nothing as before.

This change affects the test case for the ‘flush’ event.

To maintain the same test behavior as before, I switched it to just warn about unhandled requests.
@cre8ivejp what do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

What other options do we have?
Can we rewrite the test or use another tool to avoid working around it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

here is the options

"warn" (Default) Print a warning but perform the request as-is.
"error" Print an error and halt request execution.
"bypass" Does not print anything and perform the request as-is.

======================

If we want to keep using the error option, We could update the flush test case @cre8ivejp

Copy link
Collaborator Author

@duyhungtnn duyhungtnn Nov 18, 2024

Choose a reason for hiding this comment

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

I restored the code be3d641

The unhandled request was created because the cross-fetch was not working as expected.
We don't need to change any.

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch!

FakeClock,
} from './utils'
import fetch from 'cross-fetch'
// import fetch from 'cross-fetch'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cross-fetch is outdated and isn't getting any update more than a years.

since Fetch is widely available (at least from Node.js 18 and higher).
so we could get rid of "cross-fetch" as a dependency.

I am testing more to make sure. So far all tests passed when run on both browser & node environment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tested with node 18 and higher and it seems good.

mswjs example for vitest could run on both browser & node environment without cross-fetch
https://github.com/mswjs/examples/tree/main/examples/with-vitest

[StrictRequest<GetEvaluationsRequest>],
void
>()
const requestInterceptor = vi.fn((_request: StrictRequest<GetEvaluationsRequest>) => {})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

requestInterceptor should look like this with new mswjs version

@duyhungtnn duyhungtnn requested a review from cre8ivejp November 18, 2024 07:02
@duyhungtnn duyhungtnn marked this pull request as draft November 18, 2024 07:03
@duyhungtnn duyhungtnn marked this pull request as ready for review November 18, 2024 07:30
@cre8ivejp cre8ivejp changed the title fix: test fail after update non-major deps test: unit test fails when using after msw 2.6.x Nov 18, 2024
cre8ivejp
cre8ivejp previously approved these changes Nov 18, 2024
Copy link
Member

@cre8ivejp cre8ivejp left a comment

Choose a reason for hiding this comment

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

Thank you 🎉

@duyhungtnn
Copy link
Collaborator Author

I removed cross-fetch 55e737d

@cre8ivejp cre8ivejp merged commit c82a1d8 into main Nov 18, 2024
3 checks passed
@cre8ivejp cre8ivejp deleted the fix/test-fail-after-bump-deps branch November 18, 2024 10:01
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.

2 participants