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

[APM] API Snapshot Testing #77229

Merged
merged 12 commits into from
Sep 14, 2020
Merged

Conversation

dgieselaar
Copy link
Member

@dgieselaar dgieselaar commented Sep 10, 2020

Enables snapshot testing for APM API tests.

expectSnapshot(received).toMatch(): stores snapshot to file
expectSnapshot(received).toMatchInline(): stores snapshot inline

  • Titles from snapshots strive to be unique per-file. This allows non-inline snapshot tests to be shared between configurations to compare results.
  • Any remaining obsolete snapshots will fail the test.

Update all snapshots (and remove obsolete snapshots) by passing UPDATE_SNAPSHOTS=1 as an environment variable.

I've also taken the liberty of moving all literal value tests and JSON responses to (inline) snapshots.

Output from a failed test:

image

Inline snapshot:
image

@dgieselaar dgieselaar added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 10, 2020
Comment on lines 11 to 12
beforeEach(matchSnapshot.init);
afterEach(matchSnapshot.teardown);
Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively, we can try using require('../../common/match_snapshot'), and then calling beforeEach() and afterEach() in that file. We could also add an after() hook there that checks if there are unused snapshots (we'd need to track them in expectMatchSnapshot).

);

expect(responseWithoutSamples).to.eql(expectedTracesWithoutSamples);
expectSnapshot(responseWithoutSamples).toMatchInline(`
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll probably remove this later, this is just so we can see what it would mean to use inline snapshots for bigger responses.

Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced it with a more realistic scenario.

@dgieselaar dgieselaar requested a review from a team September 12, 2020 14:15
@@ -19,13 +19,19 @@ export default function ApiTest({ getService }: FtrProviderContext) {

describe('Top traces', () => {
describe('when data is not loaded ', () => {
it('handles empty state', async () => {
it('handles empty state', async function () {
Copy link
Member

Choose a reason for hiding this comment

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

Is this change necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

no longer, left over from initial exploration. I'll remove it.

Comment on lines 93 to 107
expectSnapshot({
title,
color,
type,
hideLegend,
legendValue,
}).toMatchInline(`
Object {
"color": "#54b399",
"hideLegend": false,
"legendValue": "100%",
"title": "app",
"type": "areaStacked",
}
`);
Copy link
Member

@sorenlouv sorenlouv Sep 14, 2020

Choose a reason for hiding this comment

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

Any advantages to grouping the values over doing separate expectations?
asking mostly because you've been splitting objects into multiple expectations elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I prefer the latter approach. This was also just a first iteration that I neglected to update. I'll replace it.

Comment on lines 50 to 54
it('has the correct start date', () => {
expectSnapshot(first(errorRateResponse.erroneousTransactionsRate)?.x).toMatchInline(
`1598439600000`
);
});
Copy link
Member

@sorenlouv sorenlouv Sep 14, 2020

Choose a reason for hiding this comment

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

Would it make sense to convert the timestamp to a human readable date?

Suggested change
it('has the correct start date', () => {
expectSnapshot(first(errorRateResponse.erroneousTransactionsRate)?.x).toMatchInline(
`1598439600000`
);
});
it('has the correct start date', () => {
expectSnapshot(new Date(first(errorRateResponse.erroneousTransactionsRate)?.x).toISOString()).toMatchInline(
`2020-08-26T11:00:00.000Z`
);
});

Copy link
Member Author

Choose a reason for hiding this comment

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

great idea 👍🏻

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

This is truly awesome!! Very excited to see how this works in practice.

Terminology: we cannot call the es archives "snapshots". It's just going to be too confusing 😂

  • es archives 👍
  • jest snapshots 👍
  • es snapshots 👎
  • jest archives 👎👎👎

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@dgieselaar dgieselaar merged commit 8a898fe into elastic:master Sep 14, 2020
@dgieselaar dgieselaar deleted the api-snapshot-testing branch September 14, 2020 11:51
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 14, 2020
* master: (26 commits)
  updating datatable type (elastic#77320)
  [ML] Fix custom URLs processing for security app (elastic#76957)
  [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747)
  [ML] Transforms: API schemas and integration tests (elastic#75164)
  [Mappings editor] Add support for wildcard field type (elastic#76574)
  [Ingest Manager] Fix flyout instruction selection (elastic#77071)
  [Telemetry Tools] update lodash to 4.17 (elastic#77317)
  [APM] Service inventory redesign (elastic#76744)
  Hide management sections based on cluster/index privileges (elastic#67791)
  [Snapshot Restore] Disable steps when form is invalid (elastic#76540)
  [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824)
  Update apm.ts (elastic#77310)
  [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164)
  [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986)
  [APM] API Snapshot Testing (elastic#77229)
  [ML] Functional tests - increase wait time for DFA start (elastic#77307)
  [UiActions][Drilldowns] Fix actions sorting in context menu (elastic#77162)
  [Drilldowns] Wire up new links to new docs (elastic#77154)
  Fix APM issue template
  [Ingest Pipelines] Drop into an empty tree (elastic#76885)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 14, 2020
* master: (65 commits)
  [Security Solution][Resolver] Analyzed event styling (elastic#77115)
  filter invalid SOs from the searc hresults in Task Manager (elastic#76891)
  [RUM Dashboard] Visitors by region map (elastic#77135)
  [Security Solution][Endpoint][Admin] Task/endpoint list actions (elastic#76555)
  [Ingest pipelines] Forms for processors T-U (elastic#76710)
  updating datatable type (elastic#77320)
  [ML] Fix custom URLs processing for security app (elastic#76957)
  [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747)
  [ML] Transforms: API schemas and integration tests (elastic#75164)
  [Mappings editor] Add support for wildcard field type (elastic#76574)
  [Ingest Manager] Fix flyout instruction selection (elastic#77071)
  [Telemetry Tools] update lodash to 4.17 (elastic#77317)
  [APM] Service inventory redesign (elastic#76744)
  Hide management sections based on cluster/index privileges (elastic#67791)
  [Snapshot Restore] Disable steps when form is invalid (elastic#76540)
  [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824)
  Update apm.ts (elastic#77310)
  [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164)
  [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986)
  [APM] API Snapshot Testing (elastic#77229)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 14, 2020
* master: (65 commits)
  [Security Solution][Resolver] Analyzed event styling (elastic#77115)
  filter invalid SOs from the searc hresults in Task Manager (elastic#76891)
  [RUM Dashboard] Visitors by region map (elastic#77135)
  [Security Solution][Endpoint][Admin] Task/endpoint list actions (elastic#76555)
  [Ingest pipelines] Forms for processors T-U (elastic#76710)
  updating datatable type (elastic#77320)
  [ML] Fix custom URLs processing for security app (elastic#76957)
  [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747)
  [ML] Transforms: API schemas and integration tests (elastic#75164)
  [Mappings editor] Add support for wildcard field type (elastic#76574)
  [Ingest Manager] Fix flyout instruction selection (elastic#77071)
  [Telemetry Tools] update lodash to 4.17 (elastic#77317)
  [APM] Service inventory redesign (elastic#76744)
  Hide management sections based on cluster/index privileges (elastic#67791)
  [Snapshot Restore] Disable steps when form is invalid (elastic#76540)
  [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824)
  Update apm.ts (elastic#77310)
  [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164)
  [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986)
  [APM] API Snapshot Testing (elastic#77229)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants