Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
test(integrations): Add unit tests for CaptureConsole #4733
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
test(integrations): Add unit tests for CaptureConsole #4733
Changes from all commits
9ab350e
7165018
5c24751
35d6508
179d8d8
af223e9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid the
any
cast here?What do you think about extracting the following into a helper instead just having the helper for mocking the hub? I think it works fine in the
getMockHubWithIntegration(null)
case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I don't like extracting the function we intend to test into a helper. We don't gain anything from it and the tests just become more convoluted.
As for the
any
cast: I kinda struggled with that one. TheHub
interface is huge and I didn't feel like mocking it out in its entirety when we only need a small subset. Typescript also doesn't appreciate us casting it to Hub because a bunch of methods are missing. Do you have an idea on how to do this properly?I personally really like the
jest-mock-extended
library for type-safe mocks in unit tests. It works really well, however idk if we should introduce a dependency for this. I initially discovered this library because the prisma orm recommends it for their unit tests. Let me know what you think!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah fair, let's leave the
any
cast.I would be open to adding
jest-mock-extended
, but let's stay away from it for now since we probably need to figure out how we roll it out across all of our tests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trace
should probably be patched, see #4599maybe we take that on as a quick win after this? There is also #4532 to keep in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's take this on afterwards! I'm gonna leave this test like this for now because we want to test the current behaviour. The PR that resolves #4599 should also adjust this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto for the others
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. This can't hurt. I added additional checks here: af223e9