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

[Resolver] Improve simulator. Add more click-through tests and panel tests. #74601

Merged
merged 13 commits into from
Aug 11, 2020
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
19 changes: 19 additions & 0 deletions x-pack/plugins/security_solution/common/endpoint/models/event.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,25 @@ export function timestampSafeVersion(event: SafeResolverEvent): string | undefin
: firstNonNullValue(event?.['@timestamp']);
}

/**
* The `@timestamp` for the event, as a `Date` object.
* If `@timestamp` couldn't be parsed as a `Date`, returns `undefined`.
*/
export function timestampAsDateSafeVersion(event: SafeResolverEvent): Date | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

🔴 Doc comment on export.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

const value = timestampSafeVersion(event);
if (value === undefined) {
return undefined;
}

const date = new Date(value);
// Check if the date is valid
if (isFinite(date.getTime())) {
return date;
} else {
return undefined;
}
}

export function eventTimestamp(event: ResolverEvent): string | undefined | number {
if (isLegacyEvent(event)) {
return event.endgame.timestamp_utc;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@ export function mockEndpointEvent({
parentEntityId,
timestamp,
lifecycleType,
pid = 0,
}: {
entityID: string;
name: string;
parentEntityId?: string;
timestamp: number;
lifecycleType?: string;
pid?: number;
}): EndpointEvent {
return {
'@timestamp': timestamp,
Expand All @@ -45,7 +47,7 @@ export function mockEndpointEvent({
executable: 'executable',
args: 'args',
name,
pid: 0,
pid,
hash: {
md5: 'hash.md5',
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,18 +175,21 @@ export function mockTreeWithNoAncestorsAnd2Children({
secondChildID: string;
}): ResolverTree {
const origin: ResolverEvent = mockEndpointEvent({
pid: 0,
entityID: originID,
name: 'c',
parentEntityId: 'none',
timestamp: 0,
});
const firstChild: ResolverEvent = mockEndpointEvent({
pid: 1,
entityID: firstChildID,
name: 'd',
parentEntityId: originID,
timestamp: 1,
});
const secondChild: ResolverEvent = mockEndpointEvent({
pid: 2,
entityID: secondChildID,
name: 'e',
parentEntityId: originID,
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { mount, ReactWrapper } from 'enzyme';
import { createMemoryHistory, History as HistoryPackageHistoryInterface } from 'history';
import { CoreStart } from '../../../../../../../src/core/public';
import { coreMock } from '../../../../../../../src/core/public/mocks';
import { connectEnzymeWrapperAndStore } from '../connect_enzyme_wrapper_and_store';
import { spyMiddlewareFactory } from '../spy_middleware_factory';
import { resolverMiddlewareFactory } from '../../store/middleware';
import { resolverReducer } from '../../store/reducer';
Expand Down Expand Up @@ -48,6 +47,7 @@ export class Simulator {
dataAccessLayer,
resolverComponentInstanceID,
databaseDocumentID,
history,
}: {
/**
* A (mock) data access layer that will be used to create the Resolver store.
Expand All @@ -61,6 +61,7 @@ export class Simulator {
* a databaseDocumentID to pass to Resolver. Resolver will use this in requests to the mock data layer.
*/
databaseDocumentID?: string;
history?: HistoryPackageHistoryInterface<never>;
}) {
this.resolverComponentInstanceID = resolverComponentInstanceID;
// create the spy middleware (for debugging tests)
Expand All @@ -79,8 +80,9 @@ export class Simulator {
// Create a redux store w/ the top level Resolver reducer and the enhancer that includes the Resolver middleware and the `spyMiddleware`
this.store = createStore(resolverReducer, middlewareEnhancer);

// Create a fake 'history' instance that Resolver will use to read and write query string values
this.history = createMemoryHistory();
// If needed, create a fake 'history' instance.
// Resolver will use to read and write query string values.
this.history = history ?? createMemoryHistory();

// Used for `KibanaContextProvider`
const coreStart: CoreStart = coreMock.createStart();
Expand All @@ -95,9 +97,6 @@ export class Simulator {
databaseDocumentID={databaseDocumentID}
/>
);

// Update the enzyme wrapper after each state transition
connectEnzymeWrapperAndStore(this.store, this.wrapper);
}

/**
Expand All @@ -112,6 +111,16 @@ export class Simulator {
return this.spyMiddleware.debugActions();
}

/**
* EUI uses a component called `AutoSizer` that won't render its children unless it has sufficient size.
* This forces any `AutoSizer` instances to have a large size.
*/
private forceAutoSizerOpen() {
this.wrapper
.find('AutoSizer')
.forEach((wrapper) => wrapper.setState({ width: 10000, height: 10000 }));
}

/**
* Yield the result of `mapper` over and over, once per event-loop cycle.
* After 10 times, quit.
Expand All @@ -124,6 +133,7 @@ export class Simulator {
yield mapper();
await new Promise((resolve) => {
setTimeout(() => {
this.forceAutoSizerOpen();
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ I was thinking you'd just call this when you need it in another public method (like clickRelatedButton) rather than inlining it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Few thoughts on this:

  • AutoSizer provides behavior we don't even want. We'll probably stop using EuiSelectable in 7.10.
  • We may end up accidentally using AutoSizer in the future since we use Eui. Eui could add it in places or we could start using new Eui stuff.
  • This is only needed because of limitations with JSDOM anyway.

With the stuff above in mind:

  • I don't think devs should ever have to think about this in order to write tests
  • Tests shouldn't be coupled to this

All that being said, putting this in map seems to satisfy all the constraints. Any reason why it shouldn't be there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Map is at the center of this 'new approach' to testing. Adding something that only has to be there for some edge-case-set of tests seems like it would

  1. Make the core testing framework harder to read understand
  2. Expose the rig to other weird/unexpected side-effects... maybe for some test in the future, you want that Autosizer behaving nominally vs. being interfered with.

The general idea is that it's a necessary but ugly thing to do, so you should avoid doing it when you can - and injecting it into our Enzyme main event loop makes that hard.

I guess at the very least you should add an additional comment inside the .map so the reader can understand when/why that happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lets save this discussion into a ticket or in-code docs and defer deciding until later.

this.wrapper.update();
resolve();
}, 0);
Expand Down Expand Up @@ -174,6 +184,13 @@ export class Simulator {
);
}

/**
* The items in the submenu that is opened by expanding a node in the map.
*/
public processNodeSubmenuItems(): ReactWrapper {
return this.domNodes('[data-test-subj="resolver:map:node-submenu-item"]');
}

/**
* Return the selected node query string values.
*/
Expand Down Expand Up @@ -206,38 +223,38 @@ export class Simulator {
}

/**
* An element with a list of all nodes.
* The titles of the links that select a node in the node list view.
*/
public nodeListElement(): ReactWrapper {
return this.domNodes('[data-test-subj="resolver:node-list"]');
public nodeListNodeLinkText(): ReactWrapper {
return this.domNodes('[data-test-subj="resolver:node-list:node-link:title"]');
}

/**
* Return the items in the node list (the default panel view.)
* The icons in the links that select a node in the node list view.
*/
public nodeListItems(): ReactWrapper {
return this.domNodes('[data-test-subj="resolver:node-list:item"]');
public nodeListNodeLinkIcons(): ReactWrapper {
return this.domNodes('[data-test-subj="resolver:node-list:node-link:icon"]');
}

/**
* The element containing the details for the selected node.
* Link rendered in the breadcrumbs of the node detail view. Takes the user to the node list.
*/
public nodeDetailElement(): ReactWrapper {
return this.domNodes('[data-test-subj="resolver:node-detail"]');
public nodeDetailBreadcrumbNodeListLink(): ReactWrapper {
return this.domNodes('[data-test-subj="resolver:node-detail:breadcrumbs:node-list-link"]');
}

/**
* The details of the selected node are shown in a description list. This returns the title elements of the description list.
* The title element for the node detail view.
*/
private nodeDetailEntryTitle(): ReactWrapper {
return this.domNodes('[data-test-subj="resolver:node-detail:entry-title"]');
public nodeDetailViewTitle(): ReactWrapper {
return this.domNodes('[data-test-subj="resolver:node-detail:title"]');
}

/**
* The details of the selected node are shown in a description list. This returns the description elements of the description list.
* The icon element for the node detail title.
*/
private nodeDetailEntryDescription(): ReactWrapper {
return this.domNodes('[data-test-subj="resolver:node-detail:entry-description"]');
public nodeDetailViewTitleIcon(): ReactWrapper {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just making a note here about following the page objects pattern

Copy link
Contributor Author

@oatkiller oatkiller Aug 11, 2020

Choose a reason for hiding this comment

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

What do you mean exactly?

return this.domNodes('[data-test-subj="resolver:node-detail:title-icon"]');
}

/**
Expand All @@ -253,8 +270,14 @@ export class Simulator {
* The titles and descriptions (as text) from the node detail panel.
*/
public nodeDetailDescriptionListEntries(): Array<[string, string]> {
const titles = this.nodeDetailEntryTitle();
const descriptions = this.nodeDetailEntryDescription();
/**
* The details of the selected node are shown in a description list. This returns the title elements of the description list.
*/
const titles = this.domNodes('[data-test-subj="resolver:node-detail:entry-title"]');
Copy link
Contributor

Choose a reason for hiding this comment

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

Node-detail makes me think the information is physically tied with the node in some way in the view rather than in the panel, maybe node-panel-detail? Not really sure if that's necessarily better

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, ignore me. node-detail is fine.

/**
* The details of the selected node are shown in a description list. This returns the description elements of the description list.
*/
const descriptions = this.domNodes('[data-test-subj="resolver:node-detail:entry-description"]');
const entries: Array<[string, string]> = [];
for (let index = 0; index < Math.min(titles.length, descriptions.length); index++) {
const title = titles.at(index).text();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* 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.
*/

interface Options {
/**
* The entity_id of the selected node.
*/
selectedEntityID?: string;
}

/**
* Calculate the expected URL search based on options.
*/
export function urlSearch(resolverComponentInstanceID: string, options?: Options): string {
if (!options) {
return '';
}
const params = new URLSearchParams();
if (options.selectedEntityID !== undefined) {
params.set(`resolver-${resolverComponentInstanceID}-id`, options.selectedEntityID);
}
return params.toString();
}
14 changes: 5 additions & 9 deletions x-pack/plugins/security_solution/public/resolver/view/assets.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@
* you may not use this file except in compliance with the Elastic License.
*/

/* eslint-disable react/display-name */

import React, { memo } from 'react';
import euiThemeAmsterdamDark from '@elastic/eui/dist/eui_theme_amsterdam_dark.json';
import euiThemeAmsterdamLight from '@elastic/eui/dist/eui_theme_amsterdam_light.json';
import { htmlIdGenerator, ButtonColor } from '@elastic/eui';
import styled from 'styled-components';
import { i18n } from '@kbn/i18n';
import { useUiSetting } from '../../common/lib/kibana';
import { DEFAULT_DARK_MODE } from '../../../common/constants';
import { DEFAULT_DARK_MODE as defaultDarkMode } from '../../../common/constants';
import { ResolverProcessType } from '../types';

type ResolverColorNames =
Expand Down Expand Up @@ -141,8 +143,6 @@ const PaintServers = memo(({ isDarkMode }: { isDarkMode: boolean }) => (
</>
));

PaintServers.displayName = 'PaintServers';

/**
* Ids of symbols to be linked by <use> elements
*/
Expand Down Expand Up @@ -376,8 +376,6 @@ const SymbolsAndShapes = memo(({ isDarkMode }: { isDarkMode: boolean }) => (
</>
));

SymbolsAndShapes.displayName = 'SymbolsAndShapes';

/**
* This `<defs>` element is used to define the reusable assets for the Resolver
* It confers several advantages, including but not limited to:
Expand All @@ -386,7 +384,7 @@ SymbolsAndShapes.displayName = 'SymbolsAndShapes';
* 3. `<use>` elements can be handled by compositor (faster)
*/
const SymbolDefinitionsComponent = memo(({ className }: { className?: string }) => {
const isDarkMode = useUiSetting<boolean>(DEFAULT_DARK_MODE);
const isDarkMode = useUiSetting<boolean>(defaultDarkMode);
return (
<svg className={className}>
<defs>
Expand All @@ -397,8 +395,6 @@ const SymbolDefinitionsComponent = memo(({ className }: { className?: string })
);
});

SymbolDefinitionsComponent.displayName = 'SymbolDefinitions';

export const SymbolDefinitions = styled(SymbolDefinitionsComponent)`
position: absolute;
left: 100%;
Expand All @@ -424,7 +420,7 @@ export const useResolverTheme = (): {
nodeAssets: NodeStyleMap;
cubeAssetsForNode: (isProcessTerimnated: boolean, isProcessTrigger: boolean) => NodeStyleConfig;
} => {
const isDarkMode = useUiSetting<boolean>(DEFAULT_DARK_MODE);
const isDarkMode = useUiSetting<boolean>(defaultDarkMode);
const theme = isDarkMode ? euiThemeAmsterdamDark : euiThemeAmsterdamLight;

const getThemedOption = (lightOption: string, darkOption: string): string => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,9 @@ describe('Resolver, when analyzing a tree that has no ancestors and 2 children',
});
});

it(`should show the node list`, async () => {
await expect(simulator.map(() => simulator.nodeListElement().length)).toYieldEqualTo(1);
it(`should show links to the 3 nodes (with icons) in the node list.`, async () => {
await expect(simulator.map(() => simulator.nodeListNodeLinkText().length)).toYieldEqualTo(3);
await expect(simulator.map(() => simulator.nodeListNodeLinkIcons().length)).toYieldEqualTo(3);
});

describe("when the second child node's first button has been clicked", () => {
Expand Down Expand Up @@ -152,5 +153,20 @@ describe('Resolver, when analyzing a tree that has two related events for the or
relatedEventButtons: 1,
});
});
describe('when the related events button is clicked', () => {
beforeEach(async () => {
const button = await simulator.resolveWrapper(() =>
simulator.processNodeRelatedEventButton(entityIDs.origin)
);
if (button) {
button.simulate('click');
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ Mentioned above, but I had this as a public method on the simulator, like simulator.clickNodeRelatedEventsButton(id)

}
});
it('should open the submenu', async () => {
await expect(
simulator.map(() => simulator.processNodeSubmenuItems().map((node) => node.text()))
).toYieldEqualTo(['2 registry']);
});
});
});
});
Loading