-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Improve simulator. Add more click-through tests and panel tests. #74601
Conversation
ea3eaf9
to
d8800d9
Compare
@@ -28,7 +28,6 @@ export const CubeForProcess = memo(function CubeForProcess({ | |||
height="1.5em" | |||
viewBox="0 0 1 1" | |||
> | |||
<desc>{descriptionText}</desc> |
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.
🔴 Since we're communicating meaning with the visual presentation of those cubes, I don't think it's right to remove the desc from that without at least trying to set up some alternative accessible description.
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 restored the desc
@@ -161,7 +162,7 @@ export const ProcessDetails = memo(function ProcessDetails({ | |||
<StyledBreadcrumbs breadcrumbs={crumbs} /> | |||
<EuiSpacer size="l" /> | |||
<EuiTitle size="xs"> | |||
<h4 aria-describedby={titleId}> | |||
<h4 data-test-subj="resolver:node-detail:title" aria-describedby={titleId}> |
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 don't think that's equivalent to what we're (trying?) to communicate with the cube, is 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 describedby thing ya mean?
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 the desc
back and added some basic tests for them
@@ -124,6 +133,7 @@ export class Simulator { | |||
yield mapper(); | |||
await new Promise((resolve) => { | |||
setTimeout(() => { | |||
this.forceAutoSizerOpen(); |
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 was thinking you'd just call this when you need it in another public method (like clickRelatedButton
) rather than inlining it 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.
why?
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.
Few thoughts on this:
- AutoSizer provides behavior we don't even want. We'll probably stop using EuiSelectable in 7.10.
- We may end up accidentally using AutoSizer in the future since we use Eui. Eui could add it in places or we could start using new Eui stuff.
- This is only needed because of limitations with JSDOM anyway.
With the stuff above in mind:
- I don't think devs should ever have to think about this in order to write tests
- Tests shouldn't be coupled to this
All that being said, putting this in map
seems to satisfy all the constraints. Any reason why it shouldn't be there?
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.
Map is at the center of this 'new approach' to testing. Adding something that only has to be there for some edge-case-set of tests seems like it would
- Make the core testing framework harder to read understand
- Expose the rig to other weird/unexpected side-effects... maybe for some test in the future, you want that Autosizer behaving nominally vs. being interfered with.
The general idea is that it's a necessary but ugly thing to do, so you should avoid doing it when you can - and injecting it into our Enzyme main event loop
makes that hard.
I guess at the very least you should add an additional comment inside the .map so the reader can understand when/why that happens.
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.
lets save this discussion into a ticket or in-code docs and defer deciding until later.
simulator.processNodeRelatedEventButton(entityIDs.origin) | ||
); | ||
if (button) { | ||
button.simulate('click'); |
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.
❔ Mentioned above, but I had this as a public method on the simulator, like simulator.clickNodeRelatedEventsButton(id)
@@ -16384,7 +16384,7 @@ | |||
"xpack.securitySolution.endpoint.resolver.panel.processEventListByType.eventDescriptiveName": "{descriptor} {subject}", | |||
"xpack.securitySolution.endpoint.resolver.panel.processEventListByType.events": "イベント", | |||
"xpack.securitySolution.endpoint.resolver.panel.processEventListByType.wait": "イベントを待機しています...", | |||
"xpack.securitySolution.endpoint.resolver.panel.processListWithCounts.events": "すべてのプロセスイベント", |
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.
ℹ️ Was this automated or did you do this?
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 did it. turns out changing those things is a pain now.
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.
are they going to need to be manually changed from now on then?
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.
Yeah, this seems like a thing we should cover as a team. 1) What do you do when you change the actual semantics of a translation? Like changing "Terminated Process" to "Terminated Trigger" or something? Do you delete the label and make a new one? 2) Is it safe to reuse these if the semantics are the same?
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.
good point. not sure.
Related, we could talk as a team about how to pick IDs.
Pinging @elastic/endpoint-app-team (Feature:Resolver) |
Pinging @elastic/endpoint-data-visibility-team (Team:Endpoint Data Visibility) |
@@ -55,6 +55,21 @@ export function timestampSafeVersion(event: SafeResolverEvent): string | undefin | |||
: firstNonNullValue(event?.['@timestamp']); | |||
} | |||
|
|||
export function timestampAsDateSafeVersion(event: SafeResolverEvent): Date | undefined { |
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.
🔴 Doc comment on export.
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.
good catch
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 had one additional 🔴 for missing comments, but provided that gets taken care of I'm in with a 👍 here.
*/ | ||
private nodeDetailEntryDescription(): ReactWrapper { | ||
return this.domNodes('[data-test-subj="resolver:node-detail:entry-description"]'); | ||
public nodeDetailViewTitleIcon(): ReactWrapper { |
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.
Just making a note here about following the page objects pattern
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 do you mean exactly?
/** | ||
* The details of the selected node are shown in a description list. This returns the title elements of the description list. | ||
*/ | ||
const titles = this.domNodes('[data-test-subj="resolver:node-detail:entry-title"]'); |
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.
Node-detail makes me think the information is physically tied with the node in some way in the view rather than in the panel, maybe node-panel-detail
? Not really sure if that's necessarily better
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, ignore me. node-detail
is fine.
Reverified after latest commit: 47f0cff |
💚 Build SucceededBuild metricsasync chunks size
History
To update your PR or re-run it, just comment with: |
…tests. (elastic#74601) ### Improved the simulator. * Replace `mapStateTransitions` with `map`. The usage and interface are the same, but `map` is not dependent on redux state. This will work for parts of the app that don't use redux (aka EUI). `map` also forces any `AutoSizer` instances used by EUI to show their full contents. `AutoSizer` works but it doesn't behave as expected in JSDOM. With this hack in place, we can bypass `AutoSizer`. Going forward, we should make sure to use something other than `EuiSelectable` for the dropdowns * Removed the `connectEnzymeWrapperAndStore` test helper. The new `map` simulator method doesn't rely on redux so we no longer need this explicit sync. * The simulator can receive a memory history instance. This allows tests to pass in a precreated / controlled memory instance. Useful for testing the query string. This design is not final. Instead we could have an 'intiialHistorySearch' parameter that sets the query string on instantiation as well as 'pushHistory' and 'replaceHistory' methods? * `findInDom` is now called `domNodes`. * `processNodeElementLooksSelected` and `processNodeElementLooksUnselected` are gone. Instead use `selectedProcessNode` and `unselectedProcessNode` to find the wrappers and assert that they wrappers contain the nodes you are interested in. * Added `processNodeSubmenu` method that gets the submenu that comes up when you click the events button on a process node. * Added `nodeListElement` method. This returns the list of nodes that shows up in the panel. Name is not final. * Added `nodeListItems` method. This returns the list item elements in the node list. Name is not final. * Added `nodeListNodeLinks` method. This returns the links in the items in the node list. Name is not final. * Added `nodeDetailElement` method. This gets the element that contains details about a node. Name is not final. * Added `nodeDetailBreadcrumbNodeListLink` method. Returns the link rendered in the breadcrumbs of the node detail view. Takes the user to the node list. Name is not final. * Added `nodeDetailViewTitle` method. This returns the title of the node detail view. Name is not final. * Added `nodeDetailDescriptionListEntries` method. This returns an entries list of the details in the node detail view. Name is not final * Added `resolveWrapper` method. Pass this a function that returns a `ReactWrapper`. The method will evaluate the returned wrapper after each event loop and return it once it isn't empty. ### Improved our mocks * We had a DataAccessLayer and ResolverTree mock named 'one_ancestor_two_children` that actually had no ancestors. Renamed them to `no_ancestors_two_children`. * New DataAccessLayer mock called `noAncestorsTwoChildrenWithRelatedEventsOnOrigin` ### Added new 'clickthrough' suite test * Added new test in the 'clickthrough' suite that asserts that a user can click the 'related events' button on a node and see the list of related event categories in the submenu. ### Improved the Resolver event model * Added `timestampAsDateSafeVersion` to the event model. This gets a `Date` object for the timestamp. (We still need make it clear that this model is ResolverSpecific) ### New `urlSearch` test helper. Use `urlSearch` when testing Resolver's interaction with the browser location. It calculates the expected 'search' value based on some Resolver specific parameters. * Use this to calculate a URL and then populate the memory history with this URL. This will allow you to see if Resolver loads correctly based on the URL state. * Use this to calculate the expected URL based on Resolver's current state. ### Added new 'panel' test * If Resolver is loaded with a url search parameter that selects a node, the node's details are shown in the panel. * When a history.push occurs that sets a search parameter that selects a node, the details of that node are shown. * Check that the url search is updated when the user interacts with the panel * Check that the panel shows the correct details for a node. (except for the timestamp. See TODO) ### Changed `data-test-subj`s * Removed `resolver:panel`. This was used on a wrapper element that we expect to remove soon. * Added `resolver:node-detail:breadcrumbs:node-list-link` for the buttons in the breadcrumb in the panel. * Added `resolver:node-detail:title` for the title element in the node detail view. * Added `resolver:node-detail:entry-title` and `resolver:node-detail:entry-description` for the details shown about a process in the node detail view. * Added `resolver:node-list:node-link`. This is the link shown for each node in the node list. * added `resolver:node-list:item` to each list item in the node list view. ### Removed dead code * `map.tsx` wasn't being used. It was renamed but the old version wasn't deleted. ### Improved the node detail view * Show the timestamp for a node's process event even if the timestamp is the unix epoch. Note: this is technically a bug fix but the bug is very obscure. * Show the PID for a node's process event when the PID is 0. Note: this is a bug fix.
…nes/processor-forms-a-d * 'master' of github.com:elastic/kibana: (25 commits) [ML] Removing full lodash library imports (elastic#74742) [Search] Server strategy example (elastic#71679) [Reporting] Fix and test for Listing of Reports (elastic#74453) [maps] fix drawing shapes (elastic#74689) [Resolver] Improve simulator. Add more click-through tests and panel tests. (elastic#74601) Deprecate schema-less specs in Vega (elastic#73805) [Security Solution] Rename Administration > Hosts subtab to Endpoints (elastic#74287) Timelion deprecation doc (elastic#74508) [Functional Tests] Adds a wait time between setting the index pattern and the time field on TSVB (elastic#74736) [Lens] Add styling options for x and y axes on the settings popover (elastic#71829) [Maps] add initial location option that fits to data bounds (elastic#74583) theme function (elastic#73451) [data.ui.query] Write filters to query log from default editor. (elastic#74474) [data.search.SearchSource] Move some SearchSource dependencies to the server. (elastic#74607) [Canvas][tech-debt] Convert renderers (elastic#74134) [security solutions][lists] Adds end to end tests (elastic#74473) pluralized for occurrences vs occurrence (elastic#74564) Update links that pointed to CONTRIBUTING.md (elastic#74757) [Ingest pipelines] Implement tabs in processor flyout (elastic#74469) [Event log] Use Alerts client & Actions client when fetching these types of SOs (elastic#73257) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/field_components/text_editor.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/manage_processor_form.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/append.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/bytes.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/circle.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/common_fields/field_name_field.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/common_fields/ignore_missing_field.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/convert.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/csv.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/date.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/date_index_name.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/dissect.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/dot_expander.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/drop.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/index.ts # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/shared.ts
* master: [Vega] add functional tests for Vega visualization (elastic#74097) [ML] Removing full lodash library imports (elastic#74742) [Search] Server strategy example (elastic#71679) [Reporting] Fix and test for Listing of Reports (elastic#74453) [maps] fix drawing shapes (elastic#74689) [Resolver] Improve simulator. Add more click-through tests and panel tests. (elastic#74601) Deprecate schema-less specs in Vega (elastic#73805) [Security Solution] Rename Administration > Hosts subtab to Endpoints (elastic#74287) Timelion deprecation doc (elastic#74508)
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
…tests. (#74601) (#74791) ### Improved the simulator. * Replace `mapStateTransitions` with `map`. The usage and interface are the same, but `map` is not dependent on redux state. This will work for parts of the app that don't use redux (aka EUI). `map` also forces any `AutoSizer` instances used by EUI to show their full contents. `AutoSizer` works but it doesn't behave as expected in JSDOM. With this hack in place, we can bypass `AutoSizer`. Going forward, we should make sure to use something other than `EuiSelectable` for the dropdowns * Removed the `connectEnzymeWrapperAndStore` test helper. The new `map` simulator method doesn't rely on redux so we no longer need this explicit sync. * The simulator can receive a memory history instance. This allows tests to pass in a precreated / controlled memory instance. Useful for testing the query string. This design is not final. Instead we could have an 'intiialHistorySearch' parameter that sets the query string on instantiation as well as 'pushHistory' and 'replaceHistory' methods? * `findInDom` is now called `domNodes`. * `processNodeElementLooksSelected` and `processNodeElementLooksUnselected` are gone. Instead use `selectedProcessNode` and `unselectedProcessNode` to find the wrappers and assert that they wrappers contain the nodes you are interested in. * Added `processNodeSubmenu` method that gets the submenu that comes up when you click the events button on a process node. * Added `nodeListElement` method. This returns the list of nodes that shows up in the panel. Name is not final. * Added `nodeListItems` method. This returns the list item elements in the node list. Name is not final. * Added `nodeListNodeLinks` method. This returns the links in the items in the node list. Name is not final. * Added `nodeDetailElement` method. This gets the element that contains details about a node. Name is not final. * Added `nodeDetailBreadcrumbNodeListLink` method. Returns the link rendered in the breadcrumbs of the node detail view. Takes the user to the node list. Name is not final. * Added `nodeDetailViewTitle` method. This returns the title of the node detail view. Name is not final. * Added `nodeDetailDescriptionListEntries` method. This returns an entries list of the details in the node detail view. Name is not final * Added `resolveWrapper` method. Pass this a function that returns a `ReactWrapper`. The method will evaluate the returned wrapper after each event loop and return it once it isn't empty. ### Improved our mocks * We had a DataAccessLayer and ResolverTree mock named 'one_ancestor_two_children` that actually had no ancestors. Renamed them to `no_ancestors_two_children`. * New DataAccessLayer mock called `noAncestorsTwoChildrenWithRelatedEventsOnOrigin` ### Added new 'clickthrough' suite test * Added new test in the 'clickthrough' suite that asserts that a user can click the 'related events' button on a node and see the list of related event categories in the submenu. ### Improved the Resolver event model * Added `timestampAsDateSafeVersion` to the event model. This gets a `Date` object for the timestamp. (We still need make it clear that this model is ResolverSpecific) ### New `urlSearch` test helper. Use `urlSearch` when testing Resolver's interaction with the browser location. It calculates the expected 'search' value based on some Resolver specific parameters. * Use this to calculate a URL and then populate the memory history with this URL. This will allow you to see if Resolver loads correctly based on the URL state. * Use this to calculate the expected URL based on Resolver's current state. ### Added new 'panel' test * If Resolver is loaded with a url search parameter that selects a node, the node's details are shown in the panel. * When a history.push occurs that sets a search parameter that selects a node, the details of that node are shown. * Check that the url search is updated when the user interacts with the panel * Check that the panel shows the correct details for a node. (except for the timestamp. See TODO) ### Changed `data-test-subj`s * Removed `resolver:panel`. This was used on a wrapper element that we expect to remove soon. * Added `resolver:node-detail:breadcrumbs:node-list-link` for the buttons in the breadcrumb in the panel. * Added `resolver:node-detail:title` for the title element in the node detail view. * Added `resolver:node-detail:entry-title` and `resolver:node-detail:entry-description` for the details shown about a process in the node detail view. * Added `resolver:node-list:node-link`. This is the link shown for each node in the node list. * added `resolver:node-list:item` to each list item in the node list view. ### Removed dead code * `map.tsx` wasn't being used. It was renamed but the old version wasn't deleted. ### Improved the node detail view * Show the timestamp for a node's process event even if the timestamp is the unix epoch. Note: this is technically a bug fix but the bug is very obscure. * Show the PID for a node's process event when the PID is 0. Note: this is a bug fix. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
Improved the simulator.
mapStateTransitions
withmap
. The usage and interface are the same, butmap
is not dependent on redux state. This will work for parts of the app that don't use redux (aka EUI).map
also forces anyAutoSizer
instances used by EUI to show their full contents.AutoSizer
works but it doesn't behave as expected in JSDOM. With this hack in place, we can bypassAutoSizer
. Going forward, we should make sure to use something other thanEuiSelectable
for the dropdownsconnectEnzymeWrapperAndStore
test helper. The newmap
simulator method doesn't rely on redux so we no longer need this explicit sync.findInDom
is now calleddomNodes
.processNodeElementLooksSelected
andprocessNodeElementLooksUnselected
are gone. Instead useselectedProcessNode
andunselectedProcessNode
to find the wrappers and assert that they wrappers contain the nodes you are interested in.processNodeSubmenu
method that gets the submenu that comes up when you click the events button on a process node.nodeListElement
method. This returns the list of nodes that shows up in the panel. Name is not final.nodeListItems
method. This returns the list item elements in the node list. Name is not final.nodeListNodeLinks
method. This returns the links in the items in the node list. Name is not final.nodeDetailElement
method. This gets the element that contains details about a node. Name is not final.nodeDetailBreadcrumbNodeListLink
method. Returns the link rendered in the breadcrumbs of the node detail view. Takes the user to the node list. Name is not final.nodeDetailViewTitle
method. This returns the title of the node detail view. Name is not final.nodeDetailDescriptionListEntries
method. This returns an entries list of the details in the node detail view. Name is not finalresolveWrapper
method. Pass this a function that returns aReactWrapper
. The method will evaluate the returned wrapper after each event loop and return it once it isn't empty.Improved our mocks
that actually had no ancestors. Renamed them to
no_ancestors_two_children`.noAncestorsTwoChildrenWithRelatedEventsOnOrigin
Added new 'clickthrough' suite test
Improved the Resolver event model
timestampAsDateSafeVersion
to the event model. This gets aDate
object for the timestamp. (We still need make it clear that this model is ResolverSpecific)New
urlSearch
test helper.Use
urlSearch
when testing Resolver's interaction with the browser location. It calculates the expected 'search' value based on some Resolver specific parameters.Added new 'panel' test
Changed
data-test-subj
sresolver:panel
. This was used on a wrapper element that we expect to remove soon.resolver:node-detail:breadcrumbs:node-list-link
for the buttons in the breadcrumb in the panel.resolver:node-detail:title
for the title element in the node detail view.resolver:node-detail:entry-title
andresolver:node-detail:entry-description
for the details shown about a process in the node detail view.resolver:node-list:node-link
. This is the link shown for each node in the node list.resolver:node-list:item
to each list item in the node list view.Removed dead code
map.tsx
wasn't being used. It was renamed but the old version wasn't deleted.Improved the node detail view
TODO
nodeDetailEntryDescription
andnodeDetailEntryTitle
methods should be inlined.nodeDetailElement
method andresolver:node-detail
data test subj.nodeListElement
method andresolver:node-list
data test subjdesc
for the icons.Tickets to file
data-test-subj
strings. we have no way of finding unuseddata-test-subj
s or unused accessor methods.Checklist
Delete any items that are not applicable to this PR.
For maintainers