[Security Solution][Resolver] Graph Control Tests and Update Simulator Selectors by michaelolo24 · Pull Request #74680 · elastic/kibana

michaelolo24 · 2020-08-10T16:42:07Z

Summary

Adds tests for the graph panning and zoom controls

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2020-08-10T16:46:59Z

Pinging @elastic/endpoint-app-team (Feature:Resolver)

elasticmachine · 2020-08-10T16:47:01Z

Pinging @elastic/endpoint-data-visibility-team (Team:Endpoint Data Visibility)

oatkiller

thanks for the PR. tests look good. put in a few suggestions

oatkiller · 2020-08-10T17:01:49Z

x-pack/plugins/security_solution/public/resolver/test_utilities/simulator/index.tsx

Suggested change

this.sideEffectSimulator.controls.time = new Date().getTime() + time;

this.sideEffectSimulator.controls.time = time;

new Date isn't needed here.

oatkiller · 2020-08-10T18:40:50Z

x-pack/plugins/security_solution/public/resolver/test_utilities/simulator/index.tsx

Suggested change

public readonly sideEffectSimulator: SideEffectSimulator;

private readonly sideEffectSimulator: SideEffectSimulator;

This could be private

oatkiller · 2020-08-10T18:42:30Z

x-pack/plugins/security_solution/public/resolver/view/graph_controls.test.tsx

would you mind changing this data-test-subj to use the same format as the others?

oatkiller · 2020-08-10T18:43:56Z

x-pack/plugins/security_solution/public/resolver/view/graph_controls.test.tsx

This test might not be needed. The existence of a wrapper element isn't important to the AC of any feature and its specific to the implementation details. Maybe check that the 4 buttons exist instead?

Yea, I checked for the existence of all the functional buttons instead

oatkiller · 2020-08-10T18:46:48Z

x-pack/plugins/security_solution/public/resolver/view/graph_controls.test.tsx

could you phrase the spec like:

describe('when the user clicks the south panning button', () => { beforeEach(/* the click code does here*/) it('should show the origin node lower on the screen', () => { // the expect here

This way we can easily add more it blocks that have the same setup and we can easily tell which code sets up the condition vs which code asserts stuff

Yea, this works really well. Thanks for the tip 👍

oatkiller · 2020-08-11T16:26:30Z

x-pack/plugins/security_solution/public/resolver/view/graph_controls.test.tsx

.find will return react components as well as DOM nodes. In order to test the DOM only you need to use domNodes: https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/public/resolver/test_utilities/simulator/index.tsx#L246

Would you consider using that instead?

Also, these tests would be more resilient to change if you use .map. As it is, these pass because Resolver shows the controls immediately on the first render. If we (for example) made the controls show up after data had loaded then these tests would likely fail. They might even fail now if you make the mock data access layer resolve the API calls in the next event-loop cycle.

For that reason, would you consider using .map?

Or consider resolveWrapper (uses map indirectly)

Yea, I forgot about that concern when I wrote this tbh. Updated and refactored to use .map and domNodes. Thanks!

michaelolo24 · 2020-08-12T15:53:34Z

@elasticmachine merge upstream

elasticmachine · 2020-08-12T15:53:36Z

merge conflict between base and head

oatkiller · 2020-08-12T16:20:33Z

x-pack/plugins/security_solution/public/resolver/view/graph_controls.test.tsx

+      });
+
+      it('should show the origin node further left on the screen', async () => {
+        expect(originNode.getDOMNode()).toHaveStyle({ left: '796.93132px', top: '535.5792px' });


I think this is fine to merge but long term I have some concerns:

This test relies on react to keep the same DOM node for the origin after the user clicks the pan element. If that component got a new DOM node for some reason then originNode would point to an old element that may not be in the DOM any more.

Also this relies on the react code to update the style of the element in the same event loop cycle when the user clicks the panning button.

I think we should use the pattern of calling expect on an async iterable. How about I make those changes after you merge?

@michaelolo24 Here's a commit with my ideas: 3de7be9

Thanks, cherry-picked it in and updated the selectors based on our zoom meeting

michaelolo24 · 2020-08-13T13:55:06Z

@elasticmachine merge upstream

michaelolo24 · 2020-08-13T15:24:59Z

@elasticmachine merge upstream

…s before use. use yield-equal-to pattern in graph control tests. change graph control spec language

michaelolo24 · 2020-08-13T15:58:29Z

x-pack/plugins/security_solution/public/resolver/test_utilities/simulator/index.tsx

+   * @param entityID The entity ID of the proocess node to select in
+   * @param selector The selector for the child element of the process node
+   */
+  public processNodeChildElements(entityID: string, selector: string): ReactWrapper {


@bkimmel, thoughts? I wanted to generalize your method a bit for any other buttons or things that might be added to the process node in the future

❔ It looks good, generally. Two points:

The parameter isn't really a selector (it only feeds the data-test-subj attrib). So I'd say adjust the comment.

To make that even more clear, you could put a "ByDataTestSubj" at the end of the method name (but that's awful long)

Yea, we can talk about these patterns later today as well. Gonna merge this for now

oatkiller · 2020-08-13T16:21:27Z

x-pack/plugins/security_solution/public/resolver/view/clickthrough.test.tsx

+      await expect(
+        simulator.map(() => simulator.testSubject('resolver:node-list:node-link:title').length)
+      ).toYieldEqualTo(3);
+      await expect(


seems this is duplicated

oh i see. the old version had an expect for 'text' and an expect for 'icons'

oatkiller · 2020-08-13T16:21:54Z

x-pack/plugins/security_solution/public/resolver/view/clickthrough.test.tsx

      await expect(
        simulator.map(() => ({
-          relatedEventButtons: simulator.processNodeRelatedEventButton(entityIDs.origin).length,
+          relatedEventButtons: simulator.processNodeChildElements(


Whats the purpose of the special treatment here?

Same as below. Retain the original intention here. I could move all of the actual selector string into this test or maybe even export the processNodeElementSelector and build the string at this level. Alternatively, I could tie the actual selector with the associated entity-id (see below comment)

oatkiller · 2020-08-13T16:22:30Z

x-pack/plugins/security_solution/public/resolver/view/clickthrough.test.tsx

      beforeEach(async () => {
        const button = await simulator.resolveWrapper(() =>
-          simulator.processNodeRelatedEventButton(entityIDs.origin)
+          simulator.processNodeChildElements(entityIDs.origin, 'resolver:submenu:button')


why not just testSubject here? This couples the test to DOM structure. Is there a reason?

I was just doing a one to one replacement that generalized this test a bit more. The only way to really not have this tied to the structure of the DOM would be to replace the resolver:submenu:button with a reference to the entity-id, maybe resolver:node-${entityID}:submenu:button. Is that what you're suggesting?

Oh I see what you mean. I wasn't necessarily suggesting anything but here's my thoughts:

The user doesn't care about the DOM structure and so if we don't rely on it our tests will be less brittle.

It would be cool if we are only coupled to data-test attributes. That should make it simple to keep tests working. If we remove a wrapper div, I would hope that no tests break.

Maybe at our next office hours we can discuss those ideas more.

I think there is some semantic weight to. e.g. the button being a child / ("owned by" in the case of semantics) of the process node. I say keep it as is.

Or to put it another way: If the relationship between the node and the interactive element it's expected to contain changes in some way, I think it's good that a test picks that up.

kibanamachine · 2020-08-13T17:49:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 505a348

Build metrics

async chunks size

id	value	diff	baseline
securitySolution	7.3MB	+235.0B	7.3MB

History

💔 Build #67951 failed 07a5ce17fd58f84964b2437644a962114ab68569
💔 Build #67930 failed db589d35dffc3ce96830e284a46a6e7f360b1eb4
💔 Build #67791 failed e7827537967d9b3d6385ac1dc30f0295b2860380
💔 Build #67735 failed cf33fd5
💚 Build #67584 succeeded 621a823f23853e90f13be14627c5b042ba6d53f9

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

bkimmel · 2020-08-13T18:31:17Z

x-pack/plugins/security_solution/public/resolver/view/graph_controls.test.tsx

+
+    it('should show the origin node as larger on the screen', async () => {
+      await expect(originNodeStyle()).toYieldObjectEqualTo({
+        width: '427.7538290724795px',


ℹ️ this comes from the yarn snapshot deal?

bkimmel · 2020-08-13T18:32:59Z

x-pack/plugins/security_solution/public/resolver/view/graph_controls.tsx

              className="north-button"
-              data-test-subj="north-button"
+              data-test-subj="resolver:graph-controls:north-button"
              title="North"


🎗️ later to fix these to better things like "Pan the scene up" or something. North is not a good description for what this does.

Yea, we can talk about this during office hours :)

…r Selectors (elastic#74680) Co-authored-by: oatkiller <robert.austin@elastic.co> # Conflicts: # x-pack/plugins/security_solution/public/resolver/view/clickthrough.test.tsx

* master: (23 commits) Adding API test for custom link transaction example (elastic#74238) [Uptime] Singular alert (elastic#74659) Revert "attempt excluding a codeowners directory" (elastic#75023) [Metrics UI] Remove TSVB dependency from Metrics Explorer APIs (elastic#74804) Remove degraded state from ES status service (elastic#75007) [Reporting/Functional] unskip pagination test (elastic#74973) [Resolver] Stale query string values are removed when resolver's component instance ID changes. (elastic#74979) Add public url to Workplace Search plugin (elastic#74991) [Reporting] Update more Server Types for TaskManager (elastic#74915) [I18n] verify select icu-message options are in english (elastic#74963) Make data.search.aggs available on the server. (elastic#74472) [Security Solution][Resolver] Graph Control Tests and Update Simulator Selectors (elastic#74680) attempt excluding a codeowners directory [ML] DF Analytics: allow failed job to be stopped by force via the UI (elastic#74710) Add kibana-core-ui-designers team (elastic#74970) [Metrics UI] Fix inventory footer misalignment (elastic#74707) Remove legacy optimizer (elastic#73154) Update design-specific GH code-owners (elastic#74877) skip test Reporting paginates content elastic#74922 [Metrics UI] Add Jest tests for alert previews (elastic#74890) ...

…mulator Selectors (#74680) (#75008) Co-authored-by: oatkiller <robert.austin@elastic.co>

michaelolo24 added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Data Visibility Team managing the endpoint resolver Feature:Resolver Security Solution Resolver feature v7.10.0 labels Aug 10, 2020

michaelolo24 marked this pull request as ready for review August 10, 2020 16:46

michaelolo24 requested review from a team as code owners August 10, 2020 16:46

oatkiller reviewed Aug 10, 2020

View reviewed changes

oatkiller reviewed Aug 11, 2020

View reviewed changes

michaelolo24 force-pushed the graph-control-tests branch from 621a823 to cf33fd5 Compare August 12, 2020 15:57

oatkiller reviewed Aug 12, 2020

View reviewed changes

oatkiller mentioned this pull request Aug 12, 2020

Graph control tests more #74884

Closed

7 tasks

michaelolo24 changed the title ~~[Security Solution][Resolver] Graph control tests~~ [Security Solution][Resolver] Graph Control Tests and Update Simulator Selectors Aug 12, 2020

michaelolo24 force-pushed the graph-control-tests branch from db589d3 to e782753 Compare August 13, 2020 15:23

michaelolo24 and others added 9 commits August 13, 2020 11:51

initialized graph control tests

a0aa654

added zoom tests

72ad3d4

fix comments

57eac4b

make graph control tests more explicit

9a84f5a

refactor to use domNodes and map

96be89e

slider, not button

e1b9523

Add .toYieldObjectEqualTo matcher. resolve graph control test wrapper…

f3376f8

…s before use. use yield-equal-to pattern in graph control tests. change graph control spec language

simplify selector logic

df4ca51

replaced old method references

505a348

michaelolo24 force-pushed the graph-control-tests branch from 07a5ce1 to 505a348 Compare August 13, 2020 15:56

michaelolo24 commented Aug 13, 2020

View reviewed changes

oatkiller reviewed Aug 13, 2020

View reviewed changes

oatkiller approved these changes Aug 13, 2020

View reviewed changes

bkimmel reviewed Aug 13, 2020

View reviewed changes

bkimmel approved these changes Aug 13, 2020

View reviewed changes

michaelolo24 merged commit c34e30e into elastic:master Aug 13, 2020

michaelolo24 deleted the graph-control-tests branch August 13, 2020 18:55

michaelolo24 mentioned this pull request Aug 13, 2020

[7.x] [Security Solution][Resolver] Graph Control Tests and Update Simulator Selectors (#74680) #75008

Merged

michaelolo24 added a commit that referenced this pull request Aug 14, 2020

[7.x] [Security Solution][Resolver] Graph Control Tests and Update Si…

54024de

…mulator Selectors (#74680) (#75008) Co-authored-by: oatkiller <robert.austin@elastic.co>

	this.sideEffectSimulator.controls.time = new Date().getTime() + time;
	this.sideEffectSimulator.controls.time = time;

	public readonly sideEffectSimulator: SideEffectSimulator;
	private readonly sideEffectSimulator: SideEffectSimulator;

Conversation

michaelolo24 commented Aug 10, 2020

Summary

Checklist

Uh oh!

elasticmachine commented Aug 10, 2020

Uh oh!

elasticmachine commented Aug 10, 2020

Uh oh!

oatkiller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelolo24 commented Aug 12, 2020

Uh oh!

elasticmachine commented Aug 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelolo24 commented Aug 13, 2020

Uh oh!

michaelolo24 commented Aug 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Aug 13, 2020

💚 Build Succeeded

Build metrics

async chunks size