test(integrations): Add unit tests for CaptureConsole #4733
Conversation
size-limit report
|
lforst
force-pushed
the
lforst-captureconsole-unit-tests
branch
from
ee0d41f to
9ab350e
Compare
AbhiPrasad
changed the title
AbhiPrasad
changed the title
There was a problem hiding this comment.
Congrats on this first PR! These tests look good to me. Learnt a ton about this integration when reading the tests. While these changes LGTM, I think someone who knows more about the integration should approve it, just to be extra sure about their correctness.
I was just wondering if we should perhaps test for correct behaviour when users pass an empty array for the log levels to the CaptureConsole ctor? Not sure if this adds value but it might cover a rare edge case.
|
@Lms24 Thanks for taking a look!
We have this test https://github.com/getsentry/sentry-javascript/pull/4733/files#diff-ff37e9990ef1d25deeae200c9e7e5be462f00528070535f97e9b1a2075690c8eR47-R65 but you're probably right, we should add at least one that tests the happy path for no constructor args. I added some in: 5c24751 |
Thanks, I saw the test - great to have this one for sure. What I was initially thinking about though, was having an additional test covering an edge case where you pass something like this to the constructor: |
Ohhh right. Yeah agree. I'll add another one 😄 |
| const captureConsoleIntegration = new CaptureConsole({ levels: ['log', 'warn'] }); | ||
| captureConsoleIntegration.setupOnce( | ||
| () => undefined, | ||
| () => getMockHubWithIntegration(captureConsoleIntegration) as any, |
There was a problem hiding this comment.
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.
captureConsoleIntegration.setupOnce(
() => undefined,
() => getMockHubWithIntegration(null) as any, // simulate not having the integration registered
);There was a problem hiding this comment.
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. The Hub 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.
As for the any cast: I kinda struggled with that one. The Hub 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?
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.
| expect(global.console.assert).not.toBe(originalConsole.assert); | ||
|
|
||
| // any other fields should not have been patched | ||
| expect(global.console.trace).toBe(originalConsole.trace); |
There was a problem hiding this comment.
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.
| Object.assign(global.console, originalConsole); | ||
| }); | ||
|
|
||
| it('should respect user-provided console levels', () => { |
There was a problem hiding this comment.
don't know if it can respect :P
| it('should respect user-provided console levels', () => { | |
| it('should patch user-configured console levels', () => { |
There was a problem hiding this comment.
Thanks! I updated the test name: 179d8d8
| global.console.assert(1 + 1 === 3); | ||
|
|
||
| expect(mockScope.setExtra).toHaveBeenLastCalledWith('arguments', []); | ||
| expect(mockHub.captureMessage).toHaveBeenCalledWith('Assertion failed: console.assert'); |
There was a problem hiding this comment.
ditto for the others
| expect(mockHub.captureMessage).toHaveBeenCalledWith('Assertion failed: console.assert'); | |
| expect(mockHub.captureMessage).toHaveBeenCalledTimes(1); | |
| expect(mockHub.captureMessage).toHaveBeenCalledWith('Assertion failed: console.assert'); |
There was a problem hiding this comment.
You're right. This can't hurt. I added additional checks here: af223e9
| const captureConsoleIntegration = new CaptureConsole({ levels: ['log', 'warn'] }); | ||
| captureConsoleIntegration.setupOnce( | ||
| () => undefined, | ||
| () => getMockHubWithIntegration(captureConsoleIntegration) as any, |
There was a problem hiding this comment.
As for the any cast: I kinda struggled with that one. The Hub 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?
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.
This adds some unit tests for the
captureconsoleintegration.Related to #4506
Ref: https://getsentry.atlassian.net/browse/WEB-636