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 simulator and click through tests #73310

Merged
merged 32 commits into from
Jul 29, 2020

Conversation

oatkiller
Copy link
Contributor

@oatkiller oatkiller commented Jul 27, 2020

Summary

Write a few jest tests for resolver's react code.

based on: #72791

Unfortunately it's very very slow
image

it still works

Checklist

For maintainers

Copy link
Contributor Author

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

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

still proof reading this myself. will finish up after lunch.

@@ -144,7 +144,8 @@ export function processPath(passedEvent: ResolverEvent): string | undefined {
*/
export function userInfoForProcess(
passedEvent: ResolverEvent
): { user?: string; domain?: string } | undefined {
// TODO, fix in 7.9
): { name?: string; domain?: string } | undefined {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@james-elastic seems like these values are wrong (and wont work in the UI) in 7.9. not a 100% but we should look into it

Copy link
Contributor

@jonathan-buttner jonathan-buttner Jul 28, 2020

Choose a reason for hiding this comment

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

The change you're proposing is correct. The data that is sent by the endpoint looks like this:

Endpoint process event
{
  ...
"event": {
    "sequence": 81869,
    "ingested": "2020-07-28T18:10:08.032404Z",
    "created": "2020-07-28T18:07:14.30791200Z",
    "kind": "event",
    "module": "endpoint",
    "action": "end",
    "id": "LlVyJJ01mAI5jUhA+++/7VTY",
    "category": [
      "process"
    ],
    "type": [
      "end"
    ],
    "dataset": "endpoint.events.process"
  },
  "dataset": {
    "name": "endpoint.events.process",
    "namespace": "default",
    "type": "logs"
  },
  "user": { <--------------------------------
    "domain": "NT AUTHORITY",
    "name": "LOCAL SERVICE"
  },
  "_index": ".ds-logs-endpoint.events.process-default-000001",
  "_type": "_doc",
  "_id": "ZpqelnMBlglBfzvyECQh",
  "_score": 1
}

The fields that are defined in the mapping are user.id, user.name, and user.domain: https://github.com/elastic/endpoint-package/blob/master/schemas/v1/process/process.yaml#L1260

Copy link
Contributor

Choose a reason for hiding this comment

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

@oatkiller @jonathan-buttner Should pull this (and the other user fix) out into its own PR to get it in the BC today?

@oatkiller oatkiller added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Jul 27, 2020
selector += `[data-test-resolver-node-id="${entityID}"]`;
}
if (selected) {
selector += '[aria-selected="true"]';
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ Seems like this would break if more than one thing was aria-selected wouldn't 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.

it gets added to baseResolverSelector which is '[data-test-subj="resolver:node"]' which is intended to be unique ish. although that'll break as well w/ more than 1 resolver on the page. we should probably add the document location ID to our data-test-subjs

Copy link
Contributor

@michaelolo24 michaelolo24 Jul 27, 2020

Choose a reason for hiding this comment

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

@bkimmel, when/why would we have more than one process node selected aside from 2 resolvers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we did have more than 1 process selected in a single resolver, this would return both, which would be what the test code would want (so we can say 'expected 1, got 2') or whatever

Copy link
Contributor

Choose a reason for hiding this comment

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

@michaelolo24 I was just thinking multi-resolver, mainly

/**
* Test a Resolver instance using jest, enzyme, and a mock data layer.
*/
export class Simulator {
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ this looks cool

@@ -45,7 +45,7 @@ const StyledElapsedTime = styled.div<StyledElapsedTime>`
left: ${(props) => `${props.leftPct}%`};
padding: 6px 8px;
border-radius: 999px; // generate pill shape
transform: translate(-50%, -50%);
transform: translate(-50%, -50%) rotateX(35deg);

This comment was marked as resolved.

import { SpyMiddleware, SpyMiddlewareStateActionPair } from '../types';

// TODO, rename file
export const spyMiddlewareFactory: () => SpyMiddleware = () => {

This comment was marked as resolved.

resolverComponentInstanceID: string;
}

export interface SpyMiddlewareStateActionPair {

This comment was marked as resolved.

const userEntry = {
title: 'user.name',
// TODO, bug needs to be fixed in 7.9
description: userInfoForProcess(processEvent)?.name,
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 this is a 7.9 bug

const domainEntry = {
title: 'user.domain',
// TODO, bug needs to be fixed in 7.9
description: userInfoForProcess(processEvent)?.domain,
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 this is a 7.9 bug

// eslint-disable-next-line @typescript-eslint/no-explicit-any
(animationTarget.current as any).beginElement();
if (animationTarget.current?.beginElement) {
animationTarget.current.beginElement();
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 isn't available in jsdom so i added a guard in here. shouldn't hurt anything in production

@@ -297,7 +295,8 @@ const UnstyledProcessEventDot = React.memo(
*/
return (
<div
data-test-subj={'resolverNode'}
data-test-subj="resolver:node"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

future work: make these IDs unique to each document location ID so we can test 2 resolvers at once.

* The highest level connected Resolver component. Needs a `Provider` in its ancestry to work.
*/
export const ResolverWithoutProviders = React.memo(
React.forwardRef(function (

This comment was marked as resolved.

@@ -0,0 +1,136 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

formerly map.tsx. i changed a few things but its mostly whitespace. github isn't behaving, but git can:
via git diff -w upstream/master:./x-pack/plugins/security_solution/public/resolver/view/map.tsx ./x-pack/plugins/security_solution/public/resolver/view/resolver_without_providers.tsx

diff --git a/x-pack/plugins/security_solution/public/resolver/view/map.tsx b/x-pack/plugins/security_solution/public/resolver/view/resolver_without_providers.tsx
index 30aa4b63a13..8813e09932a 100644
--- a/x-pack/plugins/security_solution/public/resolver/view/map.tsx
+++ b/x-pack/plugins/security_solution/public/resolver/view/resolver_without_providers.tsx
@@ -8,7 +8,7 @@
 
 /* eslint-disable react/display-name */
 
-import React, { useContext } from 'react';
+import React, { useContext, useCallback } from 'react';
 import { useSelector } from 'react-redux';
 import { useEffectOnce } from 'react-use';
 import { EuiLoadingSpinner } from '@elastic/eui';
@@ -24,30 +24,16 @@ import { useResolverQueryParams } from './use_resolver_query_params';
 import { StyledMapContainer, StyledPanel, GraphContainer } from './styles';
 import { entityId } from '../../../common/endpoint/models/event';
 import { SideEffectContext } from './side_effect_context';
+import { ResolverProps } from '../types';
 
 /**
  * The highest level connected Resolver component. Needs a `Provider` in its ancestry to work.
  */
-export const ResolverMap = React.memo(function ({
-  className,
-  databaseDocumentID,
-  resolverComponentInstanceID,
-}: {
-  /**
-   * Used by `styled-components`.
-   */
-  className?: string;
-  /**
-   * The `_id` value of an event in ES.
-   * Used as the origin of the Resolver graph.
-   */
-  databaseDocumentID?: string;
-  /**
-   * A string literal describing where in the app resolver is located,
-   * used to prevent collisions in things like query params
-   */
-  resolverComponentInstanceID: string;
-}) {
+export const ResolverWithoutProviders = React.memo(
+  React.forwardRef(function (
+    { className, databaseDocumentID, resolverComponentInstanceID }: ResolverProps,
+    refToForward
+  ) {
     /**
      * This is responsible for dispatching actions that include any external data.
      * `databaseDocumentID`
@@ -63,12 +49,28 @@ export const ResolverMap = React.memo(function ({
       selectors.visibleNodesAndEdgeLines
     )(timeAtRender);
     const terminatedProcesses = useSelector(selectors.terminatedProcesses);
-  const { projectionMatrix, ref, onMouseDown } = useCamera();
+    const { projectionMatrix, ref: cameraRef, onMouseDown } = useCamera();
+
+    const ref = useCallback(
+      (element: HTMLDivElement | null) => {
+        // Supply `useCamera` with the ref
+        cameraRef(element);
+
+        // If a ref is being forwarded, populate that as well.
+        if (typeof refToForward === 'function') {
+          refToForward(element);
+        } else if (refToForward !== null) {
+          refToForward.current = element;
+        }
+      },
+      [cameraRef, refToForward]
+    );
     const isLoading = useSelector(selectors.isLoading);
     const hasError = useSelector(selectors.hasError);
     const activeDescendantId = useSelector(selectors.ariaActiveDescendant);
     const { colorMap } = useResolverTheme();
     const { cleanUpQueryParams } = useResolverQueryParams();
+
     useEffectOnce(() => {
       return () => cleanUpQueryParams();
     });
@@ -76,11 +78,11 @@ export const ResolverMap = React.memo(function ({
     return (
       <StyledMapContainer className={className} backgroundColor={colorMap.resolverBackground}>
         {isLoading ? (
-        <div className="loading-container">
+          <div data-test-subj="resolver:graph:loading" className="loading-container">
             <EuiLoadingSpinner size="xl" />
           </div>
         ) : hasError ? (
-        <div className="loading-container">
+          <div data-test-subj="resolver:graph:error" className="loading-container">
             <div>
               {' '}
               <FormattedMessage
@@ -91,6 +93,7 @@ export const ResolverMap = React.memo(function ({
           </div>
         ) : (
           <GraphContainer
+            data-test-subj="resolver:graph"
             className="resolver-graph kbn-resetFocusState"
             onMouseDown={onMouseDown}
             ref={ref}
@@ -98,7 +101,8 @@ export const ResolverMap = React.memo(function ({
             tabIndex={0}
             aria-activedescendant={activeDescendantId || undefined}
           >
-          {connectingEdgeLineSegments.map(({ points: [startPosition, endPosition], metadata }) => (
+            {connectingEdgeLineSegments.map(
+              ({ points: [startPosition, endPosition], metadata }) => (
                 <EdgeLine
                   edgeLineMetadata={metadata}
                   key={metadata.uniqueId}
@@ -106,7 +110,8 @@ export const ResolverMap = React.memo(function ({
                   endPosition={endPosition}
                   projectionMatrix={projectionMatrix}
                 />
-          ))}
+              )
+            )}
             {[...processNodePositions].map(([processEvent, position]) => {
               const processEntityId = entityId(processEvent);
               return (
@@ -127,4 +132,5 @@ export const ResolverMap = React.memo(function ({
         <SymbolDefinitions />
       </StyledMapContainer>
     );
-});
+  })
+);

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I would add a comment here that the ref callback is used in the tests etc..., even though it has more use cases than that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did

</div>
) : (
<GraphContainer
data-test-subj="resolver:graph"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added data-test-subj

const terminatedProcesses = useSelector(selectors.terminatedProcesses);
const { projectionMatrix, ref: cameraRef, onMouseDown } = useCamera();

const ref = useCallback(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ref needs more complex handling now. When we get a new dom ref, call cameraRef, which sets the useCamera ref. Then if we're dealing w/ a forward ref, handle that.

return (
<StyledMapContainer className={className} backgroundColor={colorMap.resolverBackground}>
{isLoading ? (
<div data-test-subj="resolver:graph:loading" className="loading-container">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added data-test-subj

<EuiLoadingSpinner size="xl" />
</div>
) : hasError ? (
<div data-test-subj="resolver:graph:error" className="loading-container">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added data-test-subj


const userEntry = {
title: 'user.name',
description: userInfoForProcess(processEvent)?.name,
Copy link
Contributor Author

@oatkiller oatkiller Jul 27, 2020

Choose a reason for hiding this comment

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

for this file, try disabling whitespace:
image
image

My editor switched it to unix line endings. Only 2 lines should have changed in a meaningful way.


/**
* The top level, unconnected, Resolver component.
* The `Resolver` component to use. This sets up the DataAccessLayer provider. Use `ResolverWithoutStore` in tests or in other scenarios where you want to provide a different (or fake) data access layer.
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ You mean ResolverWithoutProviders right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

graphLoadingElements: simulator.graphLoadingElement().length,
graphErrorElements: simulator.graphErrorElement().length,
}))
).toSometimesYieldEqualTo({
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ what? I think I get the part about transitioning to this, but the word sometimes is making it difficult... do we mean something like finally or even at least once?

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 need better names. was hoping to come up w/ then during an in-person code review.

Copy link
Contributor Author

@oatkiller oatkiller Jul 27, 2020

Choose a reason for hiding this comment

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

so here's my logic:
Equal has significance in jest matchers. I'm trying to express that this is using a 'deep equal' type comparison like toEqual (vs toBe.)

Sometimes is meant to evoke the array method some

Here's the basic behavior:

    // Set to true if the test passes.
    let pass: boolean = false;

    // Async iterate over the iterable
    for await (const received of receivedIterable) {
      // keep track of the last value. Used in both pass and fail messages
      lastReceived = received;
      // Use deep equals to compare the value to the expected value
      if (this.equals(received, expected)) {
        // If the value is equal, break
        pass = true;
        break;
      }
    }

So it'll pass if the async generator ever yields a matching value. If it never yields a matching value and returns, the expectation will fail. if it never yields a matching value and never returns, the test will timeout.

maybe toOnceYieldEqualTo? I do expect to have other varieties of this. a strict equal variety at minimum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

toYieldAtLeastOnce sounds good. but it would be nice if the name indicated that deep equal was being used.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I think I understand better now. For testing purposes can we put a tighter collar on it; Like we care that it yielded this at least once but I think we care even more that it yielded equal last, right? So maybe finallyYieldedEqualTo which captures the timing component, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or eventuallyYieldedEqualTo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when with toYieldEqualTo for now. Let's continue improving this after merge

*/

import { Store } from 'redux';
import { ReactWrapper } from 'enzyme';
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ link to enzyme docs?

Copy link
Contributor Author

@oatkiller oatkiller Jul 27, 2020

Choose a reason for hiding this comment

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

I put a docs link in for the update function. that has some background on why this is needed. (albeit not much)

/**
* This will yield the return value of `mapper` after each state transition. If no state transition occurs for 10 event loops in a row, this will give up.
*/
public async *mapStateTransisions<R>(mapper: () => R): AsyncIterable<R> {
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

another place where better names are needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, one quick thing here. Spelled transitions wrong 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.

NO U

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@oatkiller oatkiller Jul 27, 2020

Choose a reason for hiding this comment

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

1e7abdf 1395397 i hope you're happy

* you may not use this file except in compliance with the Elastic License.
*/

import { oneAncestorTwoChildren } from '../data_access_layer/mocks/one_ancestor_two_children';
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking forward to the 3,000 processes mock 😂


// Combining assertions here for performance. Unfortunately, Enzyme + jsdom + React is slow.
it(`should have 3 nodes, with the entityID's 'origin', 'firstChild', and 'secondChild'. 'origin' should be selected.`, async () => {
expect(simulator.processNodeElementLooksSelected(entityIDs.origin)).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

gracias

@michaelolo24
Copy link
Contributor

Looks good, just the one comment in there, but I think it would be good to run through these changes in an office hours, if not just to get a better idea of handling the state transitions and some of the promise resolution stuff. We can also help with naming :)

oatkiller added 11 commits July 28, 2020 14:43
This reverts commit 8f271d0.
* debugActions now shows correct state for each action
* change `toSometimesYieldEqualTo` to `toYieldEqualTo`
* remove code that tries to show panel
* comments
`${this.utils.matcherHint(matcherName, undefined, undefined, options)}\n\n` +
`Expected: not ${this.utils.printExpected(expected)}\n${
this.utils.stringify(expected) !== this.utils.stringify(received!)
? `Received: ${this.utils.printReceived(received)}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO, show details about last received.

/**
* This will yield the return value of `mapper` after each state transition. If no state transition occurs for 10 event loops in a row, this will give up.
*/
public async *mapStateTransitions<R>(mapper: () => 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.

better name?

return this;
},
};
simulator.controls.simulateElementResize(resolverElement, size);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO, idea for next time: simulateElementResize takes a query instead of an element reference. when responding to getBoundingBox whatever check if the element matches the query and if so return the fake size.

@oatkiller
Copy link
Contributor Author

@elasticmachine merge upstream

@oatkiller
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
securitySolution 7.3MB +2.5KB 7.3MB

History

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

@oatkiller oatkiller merged commit fea3bfc into elastic:master Jul 29, 2020
@oatkiller oatkiller deleted the resolver-simulator branch July 29, 2020 00:59
oatkiller pushed a commit to oatkiller/kibana that referenced this pull request Jul 29, 2020
Write a few jest tests for resolver's react code.
oatkiller pushed a commit that referenced this pull request Jul 29, 2020
Write a few jest tests for resolver's react code.
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.

7 participants