tests: add basic sentry tests #6308
Conversation
| }); | ||
|
|
||
| it('should skip expected errors', async () => { | ||
| Sentry.init(configPayload); |
There was a problem hiding this comment.
what is an example of an expected error?
There was a problem hiding this comment.
An audit that fails because its artifact errored. In that case we'd essentially be double reporting. The real error is the one that failed in the gatherer, so we just report that one.
There was a problem hiding this comment.
lighthouse/lighthouse-core/runner.js
Lines 269 to 274 in 3d5037a
There was a problem hiding this comment.
@hoten it's pretty much only for controlling error reporting to Sentry. Basically it means this is an error, but we expect it often enough and it might be something the user needs to fix (or in the example @patrickhulce linked, we already reported it when it was a gatherer error, no reason to report it again).
| */ | ||
| 'use strict'; | ||
|
|
||
| jest.mock('raven'); |
There was a problem hiding this comment.
what's the unmocking behavior for this? The docs don't really make it clear. Does it rely on the tests being run in different processes so that other requires won't get a mock version?
There was a problem hiding this comment.
even in the same process jest has a require sandbox that require is not real require and does auto-mocking of all kinds of things so there should be no need to unmock elsewhere (though even if it were mocked everywhere, I wouldn't be disappointed in the extra layer of assurance its not going to sentry :) )
There was a problem hiding this comment.
ah yeah, less worried about sentry (not) leaking in other tests than just how they work in general for future tests :)
|
|
||
| originalSentry = {...Sentry}; | ||
| originalMathRandom = Math.random; | ||
| ravenConfigFn = jest.fn().mockReturnValue({install: jest.fn()}); |
There was a problem hiding this comment.
can this just set these directly (raven.config = jest.fn().mockReturnValue({install: jest.fn()});) and then do the expect() checks below on the actual thing (expect(raven.config).toSomethingSomething). For whatever reason that's easier for me to parse than checking expect(ravenConfigFn)
There was a problem hiding this comment.
or from the docs it seems like you can also do raven.config.mockReturnValue({install: jest.fn()}); rather than the assignment. Does that work?
There was a problem hiding this comment.
can this just set these directly
sure, no prob
it seems like you can also do raven.config.mockReturnValue({install: jest.fn()}); rather than the assignment. Does that work?
mostly :) the auto-mock only happens when the module is required in for the test, it's not reset for every it. Rather than rely on the magic mocks, have reset methods, and do the configuration, it seemed clearer to do the explicit jest.fn for the ones we're actually making assertions about. That make sense?
| environmentData: {}, | ||
| }; | ||
|
|
||
| originalSentry = {...Sentry}; |
There was a problem hiding this comment.
seems like sentry isn't modified anywhere?
There was a problem hiding this comment.
although, overriding Math.random is also kind of horrifying :) Maybe we should extract a shouldSample() function on Sentry and replace that in this test instead
There was a problem hiding this comment.
the Sentry object is mutated everytime you call init, I'll add a comment about this.
shouldSample works for me
| }); | ||
|
|
||
| it('should skip expected errors', async () => { | ||
| Sentry.init(configPayload); |
There was a problem hiding this comment.
@hoten it's pretty much only for controlling error reporting to Sentry. Basically it means this is an error, but we expect it often enough and it might be something the user needs to fix (or in the example @patrickhulce linked, we already reported it when it was a gatherer error, no reason to report it again).
|
i have a hard time understanding codecov's math here. :) |
| @@ -37,6 +37,9 @@ const sentryDelegate = { | |||
| getContext: noop, | |||
| /** @type {(error: Error, options?: CaptureOptions) => Promise<void>} */ | |||
| captureException: async () => {}, | |||
| _shouldSample() { | |||
| return SAMPLE_RATE >= Math.random(); | |||
There was a problem hiding this comment.
heh. check out sampleRate on https://docs.sentry.io/clients/node/config/
I think they added it a while ago. I wonder why we never saw it.
Anyway, I think it's fine to use our solution for now. :)
Summary
adds a few basic tests for the sentry functionality
Related Issues/PRs
#6215