-
Notifications
You must be signed in to change notification settings - Fork 8.5k
resolver simulator and click through tests #73310
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
Conversation
oatkiller
left a comment
There was a problem hiding this 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ this looks cool
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
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
7f65b60 to
7a75012
Compare
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
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>
);
-});
+ })
+);There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added data-test-subj
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added data-test-subj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added data-test-subj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ You mean ResolverWithoutProviders right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or eventuallyYieldedEqualTo
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ link to enzyme docs?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NO U
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gracias
|
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 :) |
1395397 to
22df865
Compare
This reverts commit 8f271d00919142035f868ab469a6d2a53b56b5b4.
* debugActions now shows correct state for each action * change `toSometimesYieldEqualTo` to `toYieldEqualTo` * remove code that tries to show panel * comments
6210d93 to
12ef066
Compare
| `${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)}` |
There was a problem hiding this comment.
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> { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💚 Build SucceededBuild metricsasync chunks size
History
To update your PR or re-run it, just comment with: |
Write a few jest tests for resolver's react code.


Summary
Write a few jest tests for resolver's react code.
based on: #72791
Unfortunately it's very very slow

it still works
Checklist
For maintainers