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] SideEffectContext changes, remove @testing-library uses #81077

Merged
merged 13 commits into from
Oct 27, 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
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,7 @@ export function noAncestorsTwoChildrenWithRelatedEventsOnOrigin(): {
*/
async eventsWithEntityIDAndCategory(
entityID: string,
category: string,
after?: string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed unused param

Copy link
Contributor

Choose a reason for hiding this comment

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

@bkimmel I think we added this so we could mock the pagination functionality for the /events api. Are we still hoping to implement that?

Copy link
Contributor

Choose a reason for hiding this comment

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

category: string
): Promise<{ events: SafeResolverEvent[]; nextEvent: string | null }> {
const events =
entityID === metadata.entityIDs.origin
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
* you may not use this file except in compliance with the Elastic License.
*/

export function getUiSettings(key: string): string | undefined {
/**
* A mock for Kibana UI settings.
*/
export function uiSetting(key: string): string | undefined {
if (key === 'dateFormat') {
return 'MMM D, YYYY @ HH:mm:ss.SSS';
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,25 @@ import { DataAction } from './data/action';

/**
* When the user wants to bring a node front-and-center on the map.
* @deprecated Nodes are brought into view upon selection instead. See `appReceivedNewExternalProperties`
*/
interface UserBroughtNodeIntoView {
/**
* @deprecated Nodes are brought into view upon selection instead. See `appReceivedNewExternalProperties`
*/
readonly type: 'userBroughtNodeIntoView';
/**
* @deprecated Nodes are brought into view upon selection instead. See `appReceivedNewExternalProperties`
*/
readonly payload: {
/**
* Used to identify the node that should be brought into view.
* @deprecated Nodes are brought into view upon selection instead. See `appReceivedNewExternalProperties`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

noticed this used alongside another side effect. I'm scheduled to work on removing this.

*/
readonly nodeID: string;
/**
* The time (since epoch in milliseconds) when the action was dispatched.
* @deprecated Nodes are brought into view upon selection instead. See `appReceivedNewExternalProperties`
*/
readonly time: number;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ expect.extend({
expected: T
): Promise<{ pass: boolean; message: () => string }> {
// Used in printing out the pass or fail message
const matcherName = 'toSometimesYieldEqualTo';
const matcherName = 'toYieldEqualTo';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was copy-pasted incorrectly (sorry.)

const options: jest.MatcherHintOptions = {
comment: 'deep equality with any yielded value',
isNot: this.isNot,
Expand Down Expand Up @@ -100,9 +100,9 @@ expect.extend({
expected: T
): Promise<{ pass: boolean; message: () => string }> {
// Used in printing out the pass or fail message
const matcherName = 'toSometimesYieldEqualTo';
const matcherName = 'toYieldObjectEqualTo';
const options: jest.MatcherHintOptions = {
comment: 'deep equality with any yielded value',
comment: 'subset equality with any yielded value',
isNot: this.isNot,
promise: this.promise,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { MockResolver } from './mock_resolver';
import { ResolverState, DataAccessLayer, SpyMiddleware, SideEffectSimulator } from '../../types';
import { ResolverAction } from '../../store/actions';
import { sideEffectSimulatorFactory } from '../../view/side_effect_simulator_factory';
import { getUiSettings } from '../../mocks/get_ui_settings';
import { uiSetting } from '../../mocks/ui_setting';

/**
* Test a Resolver instance using jest, enzyme, and a mock data layer.
Expand Down Expand Up @@ -62,6 +62,13 @@ export class Simulator {
return selector;
}

/**
* The simulator returns enzyme `ReactWrapper`s from various methods. Use this predicate to determine if they are DOM nodes.
*/
public static isDOM(wrapper: ReactWrapper): boolean {
return typeof wrapper.type() === 'string';
}

constructor({
dataAccessLayer,
resolverComponentInstanceID,
Expand Down Expand Up @@ -110,7 +117,7 @@ export class Simulator {
// Used for `KibanaContextProvider`
const coreStart = coreMock.createStart();

coreStart.uiSettings.get.mockImplementation(getUiSettings);
coreStart.uiSettings.get.mockImplementation(uiSetting);

this.sideEffectSimulator = sideEffectSimulatorFactory();

Expand Down Expand Up @@ -190,7 +197,7 @@ export class Simulator {
* After 10 times, quit.
* Use this to continually check a value. See `toYieldEqualTo`.
*/
public async *map<R>(mapper: () => R): AsyncIterable<R> {
public async *map<R>(mapper: (() => Promise<R>) | (() => R)): AsyncIterable<R> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function can now take a promise factory as well.

let timeoutCount = 0;
while (timeoutCount < 10) {
timeoutCount++;
Expand Down Expand Up @@ -267,6 +274,20 @@ export class Simulator {
this.sideEffectSimulator.controls.provideAnimationFrame();
}

/**
* The last value written to the clipboard via the `SideEffectors`.
*/
public get clipboardText(): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Access the (mock) clipboard text.

return this.sideEffectSimulator.controls.clipboardText;
}

/**
* Call this to resolve the promise returned by the `SideEffectors` `writeText` method (which in production points to `navigator.clipboard.writeText`.
*/
confirmTextWrittenToClipboard(): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Writing to the clipboard is asynchronous. Calling this will resolve the next enqueue promise returned from writeTextToClipboard

this.sideEffectSimulator.controls.confirmTextWrittenToClipboard();
}

/**
* The 'search' part of the URL.
*/
Expand Down Expand Up @@ -296,13 +317,36 @@ export class Simulator {
return this.domNodes(`[data-test-subj="${selector}"]`);
}

/**
* Given a `ReactWrapper`, returns a wrapper containing immediately following `dd` siblings.
* `subject` must contain just 1 element.
*/
public descriptionDetails(subject: ReactWrapper): ReactWrapper {
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 couldn't find an easy way to find the related dd element from a dt element using enzyme's ReactWrapper. There does not seem to be any way to iterate siblings. This takes a wrapper that contains exactly one dt and returns its related dd's.

// find the associated DOM nodes, then return an enzyme wrapper that only contains those.
const subjectNode = subject.getDOMNode();
let current = subjectNode.nextElementSibling;
bkimmel marked this conversation as resolved.
Show resolved Hide resolved
const associated: Set<Element> = new Set();
// Multiple `dt`s can be associated with a set of `dd`s. Skip immediately following `dt`s.
while (current !== null && current.nodeName === 'DT') {
current = current.nextElementSibling;
}
while (current !== null && current.nodeName === 'DD') {
associated.add(current);
current = current.nextElementSibling;
}
return subject
.closest('dl')
.find('dd')
.filterWhere((candidate) => {
return associated.has(candidate.getDOMNode());
});
}

/**
* Return DOM nodes that match `enzymeSelector`.
*/
private domNodes(enzymeSelector: string): ReactWrapper {
return this.wrapper
.find(enzymeSelector)
.filterWhere((wrapper) => typeof wrapper.type() === 'string');
return this.wrapper.find(enzymeSelector).filterWhere(Simulator.isDOM);
}

/**
Expand Down Expand Up @@ -331,7 +375,7 @@ export class Simulator {
* Resolve the wrapper returned by `wrapperFactory` only once it has at least 1 element in it.
*/
public async resolveWrapper(
wrapperFactory: () => ReactWrapper,
wrapperFactory: (() => Promise<ReactWrapper>) | (() => ReactWrapper),
Copy link
Contributor Author

@oatkiller oatkiller Oct 20, 2020

Choose a reason for hiding this comment

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

This accepts a promise factory now. This allows the mapper to await on conditions before returning.

predicate: (wrapper: ReactWrapper) => boolean = (wrapper) => wrapper.length > 0
): Promise<ReactWrapper | undefined> {
for await (const wrapper of this.map(wrapperFactory)) {
Expand Down
27 changes: 27 additions & 0 deletions x-pack/plugins/security_solution/public/resolver/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -490,9 +490,26 @@ export interface SideEffectors {
* A function which returns the time since epoch in milliseconds. Injected because mocking Date is tedious.
*/
timestamp: () => number;
/**
* Use instead of `window.requestAnimationFrame`
**/
requestAnimationFrame: typeof window.requestAnimationFrame;
/**
* Use instead of `window.cancelAnimationFrame`
**/
cancelAnimationFrame: typeof window.cancelAnimationFrame;
/**
* Use instead of the `ResizeObserver` global.
*/
ResizeObserver: ResizeObserverConstructor;
/**
* Use this instead of the Clipboard API's `writeText` method.
*/
writeTextToClipboard(text: string): Promise<void>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now write to the clipboard using the side effectors

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

/**
* Use this instead of `Element.prototype.getBoundingClientRect` .
*/
getBoundingClientRect(element: Element): DOMRect;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now read bounding client rects from the side effectors as well. We no longer need to use jest.spyOn to modify global objects in order to test.

}

export interface SideEffectSimulator {
Expand All @@ -512,6 +529,16 @@ export interface SideEffectSimulator {
* Trigger `ResizeObserver` callbacks for `element` and update the mocked value for `getBoundingClientRect`.
*/
simulateElementResize: (element: Element, contentRect: DOMRect) => void;

/**
* Get the most recently written clipboard text. This is only updated when `confirmTextWrittenToClipboard` is called.
*/
clipboardText: string;

/**
* Call this to resolve the promise returned by `writeText`.
*/
confirmTextWrittenToClipboard: () => void;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Writing to the clipboard returns a promise. Each time that happens the text is enqueued to be written to the clipboard. Calling this will resolve the next promise in the queue. We should use this to test what happens when the browser hangs when copying to clipboard. This can happen when copying large stuff.

};
/**
* Mocked `SideEffectors`.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,7 @@ describe('Resolver, when analyzing a tree that has no ancestors and 2 children',
).toYieldEqualTo({ treeCount: 1, nodesOwnedByTrees: 3 });
});

it(`should show links to the 3 nodes (with icons) in the node list.`, async () => {
await expect(
simulator.map(() => simulator.testSubject('resolver:node-list:node-link:title').length)
).toYieldEqualTo(3);
it(`should show links to the 3 nodes in the node list.`, async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

correcting the description here. I found a copy of this test (copy-pasted) elsewhere. I'm not sure if the text or the test should be corrected in this case.

await expect(
simulator.map(() => simulator.testSubject('resolver:node-list:node-link:title').length)
).toYieldEqualTo(3);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,33 +4,8 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React from 'react';
/* eslint-disable react/display-name */

import { EuiCode } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import styled from 'styled-components';
import React, { memo } from 'react';

/**
* Text to use in place of an undefined timestamp value
*/

export const noTimestampRetrievedText = i18n.translate(
'xpack.securitySolution.enpdoint.resolver.panelutils.noTimestampRetrieved',
{
defaultMessage: 'No timestamp retrieved',
}
);

/**
* A bold version of EuiCode to display certain titles with
*/
export const BoldCode = styled(EuiCode)`
&.euiCodeBlock code.euiCodeBlock__code {
font-weight: 900;
}
`;

/**
* A component that renders an element with breaking opportunities (`<wbr>`s)
* spliced into text children at word boundaries.
Expand Down Expand Up @@ -61,12 +36,3 @@ export const GeneratedText = React.memo(function ({ children }) {
});
}
});

/**
* A component to keep time representations in blocks so they don't wrap
* and look bad.
*/
export const StyledTime = memo(styled('time')`
display: inline-block;
text-align: start;
`);
Loading