[Resolver] simulator tests select elements directly instead of using descendant selectors. by oatkiller · Pull Request #75058 · elastic/kibana

oatkiller · 2020-08-14T16:01:22Z

Summary

Our tests shouldn't rely on the DOM structure of Resolver (when its arbitrary) because that will make them brittle. If the user doesn't care about the DOM structure, then neither should our tests.

Note: sometimes the user does care about the DOM structure, and in those cases the tests should as well.

Checklist

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

oatkiller · 2020-08-14T16:06:18Z

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

+   * The button that opens a node's submenu.
   */
-  public processNodeChildElements(entityID: string, selector: string): ReactWrapper {
+  public processNodeSubmenuButton(


This no longer relies on the DOM structure.

Cool, I wonder if it's better to just remove this method entirely to just use the testSubject and make the processNodeSubmenuButton a separate utility function? Only rationale is keeping the simulator as generic as possible. so you would have something like expect(simulator().testSubject(processNodeSubmenuButton(entityId))).toBe(blah)...then again that looks a bit gross 🤷‍♂️

i actually considered this, but the testSubject method takes just a single test subject string while processNodeSubmenuButton involves two data-attribute selectors.

I also considered packing all of the info we need into the data-test-subj attribute.

I wasn't 100% on either of those, but I think this PR is still an improvement.

Yea, it can really go either way. We could have testSubject take an options obj that builds the selector based on what's defined a la your processNodeElementSelector. So testSubject would now take the following

{ dataTestSubj: string, dataTestResolverNodeId: string, ...potentially any other future options }

yah that might work. I first want to discuss this topic: is it important that the buttons are descendants of the node's wrapper <div />. Having methods like processNodeSubmenuButton provides a level of abstraction that will allow us to easily change our mind on that. In this PR it selects an element using two attribute selectors. If we decide that the DOM structure does matter then we could change that to use a descendant selector.

This seems to be resolved by @bkimmel's comment below. Thanks

oatkiller · 2020-08-14T16:06:48Z

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

+  /**
+   * The primary button (used to select a node) which contains a label for the node as its content.
+   */
+  public processNodePrimaryButton(


Node's have 2 buttons, one to select it, one to open its submenu. This is the one that selects it.

Same comment as above, but if we decide to stay this route, would a name like processNodeShowDetailsButton work? But then again, that could be confusing considering the process cube is also clickable as well and has the same effect?

That name could also work. We could also drop process from the name. We could also do something involving aria roles somehow. For example, if the graph has a test subj, we could select [role="treeitem"] descendants to get the node wrapper elements. We could also select some other role to get this 'primary' button perhaps. I don't know enough about it yet.

aria attributes could work. The primary reason I lean towards naming this the effect the button has is that it provides the reader an initial idea of what any related tests should be doing.

I see what you mean. Related idea: perhaps we could have a selectNodeViaButtonClick method.

oatkiller · 2020-08-14T16:07:24Z

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

          graphLoadingElements: simulator.testSubject('resolver:graph:loading').length,
          graphErrorElements: simulator.testSubject('resolver:graph:error').length,
-          originNode: simulator.processNodeElements({ entityID: entityIDs.origin }).length,
+          originNodeButton: simulator.processNodePrimaryButton(entityIDs.origin).length,


this test used to check for the presence of a div, now it checks for a button (that the user can see and interact w/)

oatkiller · 2020-08-14T16:07:41Z

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

              }}
              tabIndex={-1}
              title={eventModel.processNameSafeVersion(event)}
+              data-test-subj="resolver:node:primary-button"


thoughts on this pattern?

👍 I don't see a scenario where we add more buttons in the future, but if we do, then we can change it then. The possible future scenario I'm thinking about is something like this potentially around a process node: https://tympanus.net/codrops/2013/08/09/building-a-circular-navigation-with-css-transforms/

Probably a lot of accessibility concerns with that that I have not thought about though

elasticmachine · 2020-08-14T16:08:53Z

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

elasticmachine · 2020-08-14T16:08:53Z

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

bkimmel · 2020-08-14T16:22:22Z

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

+    /** nodeID for the related node */ entityID: string
+  ): ReactWrapper {
    return this.domNodes(
-      `${processNodeElementSelector({ entityID })} [data-test-subj="${selector}"]`


❔ This is one case where the structure of the selector before ("button that is owned by the process") was valuable I think. I don't see that reflected as much in simplifying it to "a button with an ID". Someone could move that button out from the node, changing the semantics and this test would still pass.

How exactly would removing a wrapper div change the semantics?

The wrapper gets an aria-selected attribute. is that what you're thinking about?

Even though the submenu button gets tagged with the data-test-resolver-node-id that doesn't really say the same thing as the button being contained by the node. A well-meaning dev could move that button outside the node (while finding another way to populate data-test-resolver-node-id as the test expects here and break the semantic meaning without tripping the test up at all. I don't see that as an "improvement" to what's there or easier to read. We could discuss at office hours.

I guess we could assert this "contains" stuff in the test itself, but then to Michael's point earlier, it kinda beggars the purpose of this separate method altogether.

A well-meaning dev could move that button outside the node (while finding another way to populate data-test-resolver-node-id as the test expects here

That's exactly what I'm going for. Why would we want that to break the test? Is there some observable behavior that's being broken?

📏

Post discussion at office hours: We do want to test things like that buttons are contained by certain elements (say with certain aria-roles or they have some other kind of equivalent aria- relationship like owns ) but that is perhaps better to assert in individual tests (i.e. write a test that asserts that all sumbenu button are children of a role=treeitem or have some equivalent structural relationship established via other attributes.)

That allows us to keep methods on the simulator more simple/single-purpose ("just get this element") and write tests for the things we care to assert.

we discussed this. The DOM structure is important. These abstractions are still useful. With them, not every tests needs to be coupled to the DOM structure. These methods may change in the future to rely on the DOM structure, that should be fine. Also, we will have separate tests that check the DOM structure and a11y roles.

michaelolo24 · 2020-08-14T16:54:08Z

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

              tabIndex={-1}
              title={eventModel.processNameSafeVersion(event)}
+              data-test-subj="resolver:node:primary-button"
+              data-test-resolver-node-id={nodeID}


kibanamachine · 2020-08-14T17:52:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c1c91ea

Build metrics

async chunks size

id	value	diff	baseline
securitySolution	7.2MB	+154.0B	7.2MB

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

…descendant selectors. (elastic#75058) Our tests shouldn't rely on the DOM structure of Resolver (when its arbitrary) because that will make them brittle. If the user doesn't care about the DOM structure, then neither should our tests. Note: sometimes the user does care about the DOM structure, and in those cases the tests should as well.

* master: (24 commits) [ML] Functional tests - skip regression and classification tests [Ingest Manager] fix removing ingest pipelines from elasticsearch (elastic#75092) move tests for placeholder indices to setup (elastic#75096) [jest] temporarily extend default test timeout (elastic#75118) [cli] remove reference to removed --optimize flag (elastic#75083) skip flaky suite (elastic#75044) Adding /etc/rc.d/init.d/functions to the init script when present to … (elastic#22985) [jenkins] add pipeline for hourly security solution cypress tests (elastic#75087) [Reporting/Flaky Test] Skip test for paging list of reports (elastic#75075) remove .kbn-optimizer-cache upload (elastic#75086) skip flaky suite (elastic#74814) Actions add proxy support (elastic#74289) [ILM] TS conversion of Edit policy components (elastic#74747) [Resolver] simulator tests select elements directly instead of using descendant selectors. (elastic#75058) [Enterprise Search] Add Workplace Search side navigation (elastic#74894) [Security solution] Sourcerer: Kibana index pattern selector for security views (elastic#74706) [Logs UI] Remove apollo deps from log link-to routes (elastic#74502) [Maps] add map configurations to docker list (elastic#75035) [functional test][saved objects] update tests for additional copy saved objects to space (elastic#74907) Make the alerts plugin support generics (elastic#72716) ...

…descendant selectors. (#75058) (#75084) Our tests shouldn't rely on the DOM structure of Resolver (when its arbitrary) because that will make them brittle. If the user doesn't care about the DOM structure, then neither should our tests. Note: sometimes the user does care about the DOM structure, and in those cases the tests should as well. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

oatkiller added 2 commits August 14, 2020 11:06

remove dependency on dom structure

b14a0e6

Remove dependency on DOM structure

c1c91ea

oatkiller requested review from a team as code owners August 14, 2020 16:01

oatkiller assigned bkimmel and michaelolo24 Aug 14, 2020

oatkiller added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Aug 14, 2020

oatkiller commented Aug 14, 2020

View reviewed changes

oatkiller added Feature:Resolver Security Solution Resolver feature Team:Endpoint Data Visibility Team managing the endpoint resolver labels Aug 14, 2020

bkimmel reviewed Aug 14, 2020

View reviewed changes

michaelolo24 reviewed Aug 14, 2020

View reviewed changes

bkimmel approved these changes Aug 14, 2020

View reviewed changes

michaelolo24 approved these changes Aug 14, 2020

View reviewed changes

oatkiller merged commit 459e9d6 into elastic:master Aug 14, 2020

oatkiller mentioned this pull request Aug 14, 2020

[7.x] [Resolver] simulator tests select elements directly instead of using descendant selectors. (#75058) #75084

Merged

oatkiller deleted the panel-tests branch March 31, 2022 11:02

Conversation

oatkiller commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 14, 2020

Uh oh!

elasticmachine commented Aug 14, 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!

oatkiller Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 14, 2020

💚 Build Succeeded

Build metrics

async chunks size

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

oatkiller commented Aug 14, 2020 •

edited

Loading

oatkiller Aug 14, 2020 •

edited

Loading