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.

Sign up for GitHub

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 tests select elements directly instead of using descendant selectors. #75058

Merged
merged 2 commits into from
Aug 14, 2020
+45 −31
Merged

[Resolver] simulator tests select elements directly instead of using descendant selectors. #75058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b14a0e6
remove dependency on dom structure
Aug 14, 2020
c1c91ea
Remove dependency on DOM structure
Aug 14, 2020
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
22 changes: 16 additions & 6 deletions x-pack/plugins/security_solution/public/resolver/test_utilities/simulator/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,14 +175,24 @@ export class Simulator {
}

/**
* Return an Enzyme ReactWrapper for any child elements of a specific processNodeElement
*
* @param entityID The entity ID of the proocess node to select in
* @param selector The selector for the child element of the process node
* The button that opens a node's submenu.
*/
public processNodeChildElements(entityID: string, selector: string): ReactWrapper {
public processNodeSubmenuButton(
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 no longer relies on the DOM structure.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤷‍♂️

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

❔ 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How exactly would removing a wrapper div change the semantics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@oatkiller oatkiller Aug 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

📏

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.

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 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.

`[data-test-subj="resolver:submenu:button"][data-test-resolver-node-id="${entityID}"]`
);
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 see what you mean. Related idea: perhaps we could have a selectNodeViaButtonClick method.

/** nodeID for the related node */ entityID: string
): ReactWrapper {
return this.domNodes(
`[data-test-subj="resolver:node:primary-button"][data-test-resolver-node-id="${entityID}"]`
);
}

Expand Down
32 changes: 15 additions & 17 deletions x-pack/plugins/security_solution/public/resolver/view/clickthrough.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,13 @@ describe('Resolver, when analyzing a tree that has no ancestors and 2 children',
selectedOriginCount: simulator.selectedProcessNode(entityIDs.origin).length,
unselectedFirstChildCount: simulator.unselectedProcessNode(entityIDs.firstChild).length,
unselectedSecondChildCount: simulator.unselectedProcessNode(entityIDs.secondChild).length,
processNodeCount: simulator.processNodeElements().length,
nodePrimaryButtonCount: simulator.testSubject('resolver:node:primary-button').length,
}))
).toYieldEqualTo({
selectedOriginCount: 1,
unselectedFirstChildCount: 1,
unselectedSecondChildCount: 1,
processNodeCount: 3,
nodePrimaryButtonCount: 3,
});
});

Expand All @@ -82,13 +82,14 @@ describe('Resolver, when analyzing a tree that has no ancestors and 2 children',
});

describe("when the second child node's first button has been clicked", () => {
beforeEach(() => {
// Click the first button under the second child element.
simulator
.processNodeElements({ entityID: entityIDs.secondChild })
.find('button')
.first()
.simulate('click');
beforeEach(async () => {
const button = await simulator.resolveWrapper(() =>
simulator.processNodePrimaryButton(entityIDs.secondChild)
);
// Click the second child node's primary button
if (button) {
button.simulate('click');
}
});
it('should render the second child node as selected, and the origin as not selected, and the query string should indicate that the second child is selected', async () => {
await expect(
Expand Down Expand Up @@ -141,23 +142,20 @@ describe('Resolver, when analyzing a tree that has two related events for the or
graphElements: simulator.testSubject('resolver:graph').length,
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,
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 test used to check for the presence of a div, now it checks for a button (that the user can see and interact w/)

}))
).toYieldEqualTo({
graphElements: 1,
graphLoadingElements: 0,
graphErrorElements: 0,
originNode: 1,
originNodeButton: 1,
});
});

it('should render a related events button', async () => {
await expect(
simulator.map(() => ({
relatedEventButtons: simulator.processNodeChildElements(
entityIDs.origin,
'resolver:submenu:button'
).length,
relatedEventButtons: simulator.processNodeSubmenuButton(entityIDs.origin).length,
}))
).toYieldEqualTo({
relatedEventButtons: 1,
Expand All @@ -166,7 +164,7 @@ describe('Resolver, when analyzing a tree that has two related events for the or
describe('when the related events button is clicked', () => {
beforeEach(async () => {
const button = await simulator.resolveWrapper(() =>
simulator.processNodeChildElements(entityIDs.origin, 'resolver:submenu:button')
simulator.processNodeSubmenuButton(entityIDs.origin)
);
if (button) {
button.simulate('click');
Expand All @@ -183,7 +181,7 @@ describe('Resolver, when analyzing a tree that has two related events for the or
describe('and when the related events button is clicked again', () => {
beforeEach(async () => {
const button = await simulator.resolveWrapper(() =>
simulator.processNodeChildElements(entityIDs.origin, 'resolver:submenu:button')
simulator.processNodeSubmenuButton(entityIDs.origin)
);
if (button) {
button.simulate('click');
Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/security_solution/public/resolver/view/process_event_dot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,8 @@ const UnstyledProcessEventDot = React.memo(
}}
tabIndex={-1}
title={eventModel.processNameSafeVersion(event)}
data-test-subj="resolver:node:primary-button"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thoughts on this pattern?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 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

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it

data-test-resolver-node-id={nodeID}
Copy link
Contributor

Choose a reason for hiding this comment

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

sweeet

>
<span className="euiButton__content">
<span className="euiButton__text" data-test-subj={'euiButton__text'}>
Expand Down Expand Up @@ -433,6 +435,7 @@ const UnstyledProcessEventDot = React.memo(
menuTitle={subMenuAssets.relatedEvents.title}
projectionMatrix={projectionMatrix}
optionsWithActions={relatedEventStatusOrOptions}
nodeID={nodeID}
/>
)}
</EuiFlexItem>
Expand Down
16 changes: 8 additions & 8 deletions x-pack/plugins/security_solution/public/resolver/view/query_params.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@ describe('Resolver, when analyzing a tree that has no ancestors and 2 children',

describe("when the second child node's first button has been clicked", () => {
beforeEach(async () => {
const node = await simulator.resolveWrapper(() =>
simulator.processNodeElements({ entityID: entityIDs.secondChild }).find('button')
const button = await simulator.resolveWrapper(() =>
simulator.processNodePrimaryButton(entityIDs.secondChild)
);
if (node) {
if (button) {
// Click the first button under the second child element.
node.first().simulate('click');
button.simulate('click');
}
});
const expectedSearch = urlSearch(resolverComponentInstanceID, {
Expand Down Expand Up @@ -68,12 +68,12 @@ describe('Resolver, when analyzing a tree that has no ancestors and 2 children',
});
describe("when the user clicks the second child node's button again", () => {
beforeEach(async () => {
const node = await simulator.resolveWrapper(() =>
simulator.processNodeElements({ entityID: entityIDs.secondChild }).find('button')
const button = await simulator.resolveWrapper(() =>
simulator.processNodePrimaryButton(entityIDs.secondChild)
);
if (node) {
if (button) {
// Click the first button under the second child element.
node.first().simulate('click');
button.simulate('click');
}
});
it(`should have a url search of ${urlSearch(newInstanceID, {
Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/security_solution/public/resolver/view/submenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ const NodeSubMenuComponents = React.memo(
optionsWithActions,
className,
projectionMatrix,
nodeID,
}: {
menuTitle: string;
className?: string;
Expand All @@ -148,6 +149,7 @@ const NodeSubMenuComponents = React.memo(
* Receive the projection matrix, so we can see when the camera position changed, so we can force the submenu to reposition itself.
*/
projectionMatrix: Matrix3;
nodeID: string;
} & {
optionsWithActions?: ResolverSubmenuOptionList | string | undefined;
}) => {
Expand Down Expand Up @@ -236,6 +238,7 @@ const NodeSubMenuComponents = React.memo(
iconSide="right"
tabIndex={-1}
data-test-subj="resolver:submenu:button"
data-test-resolver-node-id={nodeID}
>
{count ? <EuiI18nNumber value={count} /> : ''} {menuTitle}
</EuiButton>
Expand Down