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.

Sign up for GitHub

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

[Uptime] Add unit tests for QueryContext time calculation #56671

Merged
merged 2 commits into from
Feb 4, 2020
Merged

[Uptime] Add unit tests for QueryContext time calculation #56671

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
536662b
[Uptime] Add unit tests for QueryContext time calculation
andrewvc Feb 3, 2020
cfd819c
Incorporate PR feedback
andrewvc Feb 3, 2020
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
86 changes: 86 additions & 0 deletions .../plugins/uptime/server/lib/adapters/monitor_states/search/__tests__/query_context.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { QueryContext } from '../query_context';
import { CursorPagination } from '../..';
import { CursorDirection, SortOrder } from '../../../../../../common/graphql/types';

describe(QueryContext, () => {
// 10 minute range
const rangeStart = '2019-02-03T19:06:54.939Z';
const rangeEnd = '2019-02-03T19:16:54.939Z';

const pagination: CursorPagination = {
cursorDirection: CursorDirection.AFTER,
sortOrder: SortOrder.DESC,
};

let qc: QueryContext;
const makeQueryContext = () => {
return new QueryContext({}, rangeStart, rangeEnd, pagination, null, 10);
};
beforeEach(() => (qc = makeQueryContext()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
beforeEach(() => (qc = makeQueryContext()));
beforeEach(() => (qc = new QueryContext({}, rangeStart, rangeEnd, pagination, null, 10)));

Could we just instantiate a new context directly here, since makeQueryContext isn't referenced anywhere else in the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I left it there as a style preference, since I thought we might use it in the future, but I think you're right and YAGNI applies here.


describe('dateRangeFilter()', () => {
const expectedRange = {
range: {
'@timestamp': {
gte: rangeStart,
lte: rangeEnd,
},
},
};
describe('when hasTimespan() is true', () => {
it('should create a date range filter including the timespan', async () => {
const mockHasTimespan = jest.fn();
mockHasTimespan.mockReturnValue(Promise.resolve(true));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mockHasTimespan.mockReturnValue(Promise.resolve(true));
mockHasTimespan.mockReturnValue(true);

I don't think we need the Promise.resolve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? That function is async, so should always return a promise.

qc.hasTimespan = mockHasTimespan;

expect(await qc.dateRangeFilter()).toEqual({
bool: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This type of assertion is fine with me if we all agree to use/follow it. I personally would use a snapshot for this type of response. The spirit of the snapshot is that, I have evaluated its contents and I know it's what I want in the initial write. After that it should never change unless the module's expected output changes. It results in cleaner-looking test files IMO. But this assertion is effectively the same thing as what a snapshot does, so really it's a semantic preference more than anything else. I am fine either way; if this is more helpful for evaluating what's going on in the test we should do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think my preference goes the other way. If the output of timespanClause() changes, this test will still work and no snapshot will need to be updated. This test isn't testing timespanClause, there's a dedicated test for that, so changes to the logic there shouldn't affect the pass/fail nature of the test here.

Would you disagree that this property is a benefit?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a pretty good point, I do agree that in this case we don't necessarily want to test the output of functionality that's tested elsewhere.

filter: [
expectedRange,
{
bool: {
should: [
qc.timespanClause(),
{ bool: { must_not: { exists: { field: 'monitor.timespan' } } } },
],
},
},
],
},
});
});
});

describe('when hasTimespan() is false', () => {
it('should only use the timestamp fields in the returned filter', async () => {
const mockHasTimespan = jest.fn();
mockHasTimespan.mockReturnValue(Promise.resolve(false));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mockHasTimespan.mockReturnValue(Promise.resolve(false));
mockHasTimespan.mockReturnValue(false);

I think it's ok to just return the boolean here.

qc.hasTimespan = mockHasTimespan;

expect(await qc.dateRangeFilter()).toEqual(expectedRange);
});
});
});

describe('timespanClause()', () => {
it('should always cover the last 5m', () => {
// 5m expected range between GTE and LTE in the response
// since timespan is hardcoded to 5m
expect(qc.timespanClause()).toEqual({
range: {
'monitor.timespan': {
// end date minus 5m
gte: new Date(Date.parse(rangeEnd) - 5 * 60 * 1000).toISOString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to me like it will be ok, we're simply modifying a constant value, not computing any relative time.

I view these types of assertions with healthy skepticism always. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the idea with using math here instead of a writing it out was to communicate the semantic intent and also to allow the value at the top of the file to change without breaking the test if that's useful for a future test.

Copy link
Contributor

Choose a reason for hiding this comment

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

++ I agree with the implementation. I just look at anything with date manipulation 3x more than the rest of the code in a given diff 😆

lte: rangeEnd,
},
},
});
});
});
});