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

andrewvc merged 2 commits into elastic:master from andrewvc:add-qc-unit-tests

Feb 4, 2020

Contributor

andrewvc commented Feb 3, 2020

Add Unit tests for the QueryContext class that was missing testing. This would have caught #56612


[Uptime] Add unit tests for QueryContext time calculation

536662b

andrewvc added technical debt Team:Uptime - DEPRECATED v7.7.0 backport:skip labels

andrewvc self-assigned this

Contributor

elasticmachine commented Feb 3, 2020

Pinging @elastic/uptime (Team:uptime)

andrewvc added release_note:skip and removed backport:skip labels

justinkambic reviewed

View reviewed changes

Contributor

justinkambic left a comment

Had a few recommendations and shared my opinion about the test paradigm outlined here.

...acy/plugins/uptime/server/lib/adapters/monitor_states/search/__tests__/query_context.test.ts

+ qc.hasTimespan = mockHasTimespan;
+ expect(await qc.dateRangeFilter()).toEqual({
+ bool: {

Contributor

justinkambic Feb 3, 2020

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.

Contributor Author

andrewvc Feb 3, 2020

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?

Contributor

justinkambic Feb 3, 2020

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.

...acy/plugins/uptime/server/lib/adapters/monitor_states/search/__tests__/query_context.test.ts Outdated

+ const makeQueryContext = () => {
+ return new QueryContext({}, rangeStart, rangeEnd, pagination, null, 10);
+ };
+ beforeEach(() => (qc = makeQueryContext()));

Contributor

justinkambic Feb 3, 2020

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?

Contributor Author

andrewvc Feb 3, 2020

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.

...acy/plugins/uptime/server/lib/adapters/monitor_states/search/__tests__/query_context.test.ts

+ range: {
+ 'monitor.timespan': {
+ // end date minus 5m
+ gte: new Date(Date.parse(rangeEnd) - 5 * 60 * 1000).toISOString(),

Contributor

justinkambic Feb 3, 2020

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. 😅

Contributor Author

andrewvc Feb 3, 2020

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.

Contributor

justinkambic Feb 3, 2020

++ 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 😆

...acy/plugins/uptime/server/lib/adapters/monitor_states/search/__tests__/query_context.test.ts Outdated

+ 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));

Contributor

justinkambic Feb 3, 2020

Suggested change

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

 mockHasTimespan.mockReturnValue(true);

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

Contributor Author

andrewvc Feb 3, 2020

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

...acy/plugins/uptime/server/lib/adapters/monitor_states/search/__tests__/query_context.test.ts Outdated

+ 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));

Contributor

justinkambic Feb 3, 2020

Suggested change

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

 mockHasTimespan.mockReturnValue(false);

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


Incorporate PR feedback

cfd819c

justinkambic approved these changes

View reviewed changes

Contributor

justinkambic left a comment

LGTM

...acy/plugins/uptime/server/lib/adapters/monitor_states/search/__tests__/query_context.test.ts

+ range: {
+ 'monitor.timespan': {
+ // end date minus 5m
+ gte: new Date(Date.parse(rangeEnd) - 5 * 60 * 1000).toISOString(),

Contributor

justinkambic Feb 3, 2020

++ 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 😆

...acy/plugins/uptime/server/lib/adapters/monitor_states/search/__tests__/query_context.test.ts

+ qc.hasTimespan = mockHasTimespan;
+ expect(await qc.dateRangeFilter()).toEqual({
+ bool: {

Contributor

justinkambic Feb 3, 2020

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.

Contributor

kibanamachine commented Feb 3, 2020

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: cfd819c

History

💚 Build #24130 succeeded 536662b

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

andrewvc merged commit 653c28a into elastic:master

andrewvc deleted the add-qc-unit-tests branch

February 4, 2020 02:10

andrewvc mentioned this pull request

[7.x] [Uptime] Add unit tests for QueryContext time calculation (#56671) #56718

Merged

andrewvc added a commit to andrewvc/kibana that referenced this pull request


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

4f4430f

)

Add Unit tests for the QueryContext class that was missing testing. This would have caught elastic#56612

gmmorris added a commit to gmmorris/kibana that referenced this pull request


Merge branch 'master' into alerting/get-alert-state

28e7c7e

* master: (42 commits)
  Move kuery_autocomplete ⇒ NP (elastic#56607)
  [ML] Functional tests - stabilize job row and analytics result view assertions (elastic#56595)
  [Discover] Inline angular directives only used in this plugin (elastic#56119)
  [Discover] Migrate get_sort.js test from mocha to TypeScript (elastic#56011)
  [SIEM] Enable flow_target_select_connected unit tests (elastic#55618)
  Start consuming np logging config (elastic#56480)
  [SIEM] Add eslint-plugin-react-perf (elastic#55960)
  Mention changed SAML ACS endpoint URL in breaking changes doc. (elastic#56613)
  Add `getServerInfo` API to http setup contract (elastic#56636)
  Updates Monitoring alert Jest snapshots
  Kibana property config migrations (elastic#55937)
  Vislib replacement toggle (elastic#56439)
  [Uptime] Add unit tests for QueryContext time calculation (elastic#56671)
  [SIEM][Detection Engine] Critical blocker, fixes pre-packaged rule miscounts
  Upgrade EUI to v18.3.0 (elastic#56228)
  [Maps] Fix server log (elastic#56679)
  [SIEM] Fixes FTUE when APM node is present (elastic#56574)
  [Reporting/FieldFormats] expose `setFieldFormats` and call from ReportingPlugin.start (elastic#56563)
  Update EMS API urls for production (elastic#56657)
  Ability to delete alerts even when AAD is out of sync (elastic#56543)
  ...

andrewvc added a commit that referenced this pull request


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

abcc6b7

…56718)

Add Unit tests for the QueryContext class that was missing testing. This would have caught #56612

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

cauemarcondes pushed a commit to cauemarcondes/kibana that referenced this pull request


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

819081a

)

Add Unit tests for the QueryContext class that was missing testing. This would have caught elastic#56612

majagrubic pushed a commit to majagrubic/kibana that referenced this pull request


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

)

Add Unit tests for the QueryContext class that was missing testing. This would have caught elastic#56612

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:skip Team:Uptime - DEPRECATED technical debt v7.7.0