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

tests: add basic sentry tests #6308

Merged
merged 2 commits into from
Oct 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lighthouse-core/lib/sentry.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ const sentryDelegate = {
getContext: noop,
/** @type {(error: Error, options?: CaptureOptions) => Promise<void>} */
captureException: async () => {},
_shouldSample() {
return SAMPLE_RATE >= Math.random();
Copy link
Member

Choose a reason for hiding this comment

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

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. :)

},
};

/**
Expand All @@ -50,7 +53,7 @@ function init(opts) {
}

// If not selected for samping, leave the functions as a noop.
if (SAMPLE_RATE <= Math.random()) {
if (!sentryDelegate._shouldSample()) {
return;
}

Expand Down
126 changes: 126 additions & 0 deletions lighthouse-core/test/lib/sentry-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/**
* @license Copyright 2018 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

jest.mock('raven');
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 :) )

Copy link
Member

@brendankenny brendankenny Oct 17, 2018

Choose a reason for hiding this comment

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

ah yeah, less worried about sentry (not) leaking in other tests than just how they work in general for future tests :)


const raven = require('raven');
const Sentry = require('../../lib/sentry');

/* eslint-env jest */

describe('Sentry', () => {
let configPayload;
let originalSentry;

beforeEach(() => {
configPayload = {
url: 'http://example.com',
flags: {enableErrorReporting: true},
environmentData: {},
};

// We need to save the Sentry delegate object because every call to `.init` mutates the methods.
// We want to have a fresh state for every test.
originalSentry = {...Sentry};
Copy link
Member

Choose a reason for hiding this comment

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

seems like sentry isn't modified anywhere?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the Sentry object is mutated everytime you call init, I'll add a comment about this.

shouldSample works for me


raven.config = jest.fn().mockReturnValue({install: jest.fn()});
raven.mergeContext = jest.fn();
raven.captureException = jest.fn().mockImplementation((err, opts, cb) => cb());
Sentry._shouldSample = jest.fn().mockReturnValue(true);
});

afterEach(() => {
// Reset the methods on the Sentry object, see note above.
Object.assign(Sentry, originalSentry);
});

describe('.init', () => {
it('should noop when !enableErrorReporting', () => {
Sentry.init({url: 'http://example.com', flags: {}});
expect(raven.config).not.toHaveBeenCalled();
Sentry.init({url: 'http://example.com', flags: {enableErrorReporting: false}});
expect(raven.config).not.toHaveBeenCalled();
});

it('should noop when not picked for sampling', () => {
Sentry._shouldSample.mockReturnValue(false);
Sentry.init({url: 'http://example.com', flags: {enableErrorReporting: true}});
expect(raven.config).not.toHaveBeenCalled();
});

it('should initialize the raven client when enableErrorReporting', () => {
Sentry.init({
url: 'http://example.com',
flags: {
enableErrorReporting: true,
emulatedFormFactor: 'desktop',
throttlingMethod: 'devtools',
},
environmentData: {},
});

expect(raven.config).toHaveBeenCalled();
expect(raven.mergeContext).toHaveBeenCalled();
expect(raven.mergeContext.mock.calls[0][0]).toEqual({
extra: {
url: 'http://example.com',
deviceEmulation: true,
emulatedFormFactor: 'desktop',
throttlingMethod: 'devtools',
},
});
});
});

describe('.captureException', () => {
it('should forward exceptions to raven client', async () => {
Sentry.init(configPayload);
const error = new Error('oops');
await Sentry.captureException(error);

expect(raven.captureException).toHaveBeenCalled();
expect(raven.captureException.mock.calls[0][0]).toBe(error);
});

it('should skip expected errors', async () => {
Sentry.init(configPayload);
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is an example of an expected error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

// Create a friendlier display error and mark it as expected to avoid duplicates in Sentry
const error = new Error(
`Required ${artifactName} gatherer encountered an error: ${artifactError.message}`);
// @ts-ignore Non-standard property added to Error
error.expected = true;
throw error;

Copy link
Member

Choose a reason for hiding this comment

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

@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).

const error = new Error('oops');
error.expected = true;
await Sentry.captureException(error);

expect(raven.captureException).not.toHaveBeenCalled();
});

it('should skip duplicate audit errors', async () => {
Sentry.init(configPayload);
const error = new Error('A');
await Sentry.captureException(error, {tags: {audit: 'my-audit'}});
await Sentry.captureException(error, {tags: {audit: 'my-audit'}});

expect(raven.captureException).toHaveBeenCalledTimes(1);
});

it('should still allow different audit errors', async () => {
Sentry.init(configPayload);
const errorA = new Error('A');
const errorB = new Error('B');
await Sentry.captureException(errorA, {tags: {audit: 'my-audit'}});
await Sentry.captureException(errorB, {tags: {audit: 'my-audit'}});

expect(raven.captureException).toHaveBeenCalledTimes(2);
});

it('should skip duplicate gatherer errors', async () => {
Sentry.init(configPayload);
const error = new Error('A');
await Sentry.captureException(error, {tags: {gatherer: 'my-gatherer'}});
await Sentry.captureException(error, {tags: {gatherer: 'my-gatherer'}});

expect(raven.captureException).toHaveBeenCalledTimes(1);
});
});
});