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.

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

[Security_Solution][Resolver] Resolver loading and error state #75600

Merged
merged 12 commits into from
Aug 27, 2020

Conversation

michaelolo24
Copy link
Contributor

@michaelolo24 michaelolo24 commented Aug 20, 2020

Summary

Basic tests added for the different states of the resolver. Loading, Error, and Success.

General Changes

  • Updated the extended jest toYieldEqualTo to require a successful view state to remain successful for the remaining iterations of the event loop to be successful. I can update this to require success for x number of iterations if we decide to go that route.

  • Updated one clickthrough test that was failing based on the above

Loading Tests
This test utilizes pause functionality within the mock dataAccessLayer, which will load the resolver with configured requests in a paused state. This allows us to check that the loading state occurs and is maintained while awaiting resolution of those requests.

Error Test
This test utilizes and emptiable dataAccessLayer that allows configuration of which requests return an empty set of data. Based on the logic in resolver_tree_fetcher.ts an error will occur if no entities are returned, but if an empty tree is returned the resolver will show a blank window (this scenario should never occur since the entityID request would need to be successful for the resolverTree request to even run).

Success Test
Simple test that just makes sure all nodes loaded in the graph successfully.

Checklist

@michaelolo24 michaelolo24 force-pushed the resolver-loading-states branch 2 times, most recently from 31e59e6 to 2bf8258 Compare August 24, 2020 18:43
@michaelolo24 michaelolo24 force-pushed the resolver-loading-states branch 2 times, most recently from 0894e34 to d45a17b Compare August 24, 2020 19:47
@michaelolo24 michaelolo24 changed the title WIP: need better control flow [Security_Solution][Resolver] Resolver loading and error state Aug 24, 2020
@michaelolo24 michaelolo24 force-pushed the resolver-loading-states branch 2 times, most recently from 6d22c28 to f04ace5 Compare August 24, 2020 21:09
@michaelolo24 michaelolo24 added Feature:Resolver Security Solution Resolver feature release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Data Visibility Team managing the endpoint resolver v7.10.0 v8.0.0 labels Aug 24, 2020
@michaelolo24 michaelolo24 marked this pull request as ready for review August 24, 2020 21:16
@michaelolo24 michaelolo24 requested review from a team as code owners August 24, 2020 21:16
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

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

Added comments with a few thoughts.

// Trigger a loading state by requesting data based on a new DocumentID.
// There really is no way to do this in the view besides changing the url, so triggering the action instead

simulator.requestNewTree(newTreeId);
Copy link
Contributor

Choose a reason for hiding this comment

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

You could change the database document ID prop using setProp from enzyme. That way you don't have to have public simulator methods that rely on the internals of Resolver (I'm considering the props to the Resolver component as public.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see below comment

* Find the graph container by the database document ID.
* This better allows us to test when new data is loaded into an existing resolver
*/
public processResolverGraph(databaseDocumentID: string): ReactWrapper {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another possible approach. Create a mock data access layer that returns different data based on the databaseDocumentID. Load resolver, then change the databaseDocumentID props. New data will be loaded. You will be able to verify that based on the data in the panel and the graph. maybe?

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 like this idea, but the more I thought about it, I realized I would be testing functionality that we don't currently have in the UI, as far as I can tell, so figured it would be best to leave this till we figure out what that implementation may look like. 🤷‍♂️

});
});

describe("When the resolver tree request doesn't return any data", () => {
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 should really never happen since the resolverTree request only takes place after successfully getting an entityID. Have this test here though so we're at least checking the behavior in case something wonky happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could happen if different indices are used for entities than what are used for resolverTree. I think that might even be the case today.

}

/**
* A simple mock dataAccessLayer that allows you to configure which requests return an empty set of data.
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ good idea. These empty kinds seem to give us a lot of trouble, We should use this a lot in tests.


it('should display a loading state', async () => {
// Trigger a loading state by requesting data based on a new DocumentID.
// There really is no way to do this in the view besides changing the url, so triggering the action instead
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ Where is the action mentioned in this comment being triggered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

errant comment from an earlier implementation. Gonna remove, thanks!

}

/**
* A simple mock dataAccessLayer that allows you to manually pause and resume a request.
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ These emptiable and pausable DALs are really cool. Just wondering if there is a way we could make those concepts composable somehow, like if you wanted something to delay an empty request you could do something like Emptiable(Pausable(mockTreeWithTwoAncestors(), {relatedEvents: true}), {relatedEvent: true})

Copy link
Contributor

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

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

left a few suggestions

}

/**
* A simple mock dataAccessLayer that allows you to manually pause and resume a request.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is out of date.

async entities(): Promise<ResolverEntityIndex> {
return dataShouldBeEmpty?.includes('entities')
? Promise.resolve([])
: // @ts-ignore - ignore the argument requirement for dataAccessLayer
Copy link
Contributor

Choose a reason for hiding this comment

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

what error is happening here exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated to just pass args through.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you forgot to remove the @ts-ignore after fixing the issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Also you can switch to @ts-expect-error now

});
});

describe("When the resolver tree request doesn't return any data", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could happen if different indices are used for entities than what are used for resolverTree. I think that might even be the case today.

Copy link
Contributor

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

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

Nice

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

distributable file count

id value diff baseline
total 53128 +2 53126

History

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

@michaelolo24 michaelolo24 merged commit 280fb8b into elastic:master Aug 27, 2020
@michaelolo24 michaelolo24 deleted the resolver-loading-states branch August 27, 2020 20:42
michaelolo24 added a commit to michaelolo24/kibana that referenced this pull request Aug 27, 2020
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 31, 2020
…s-for-710

* 'master' of github.com:elastic/kibana: (43 commits)
  [APM] Chart units don't update when toggling the chart legends (elastic#74931)
  [ILM] Add support for frozen phase in UI (elastic#75968)
  Hides advanced json for count metric (elastic#74636)
  add client-side feature usage API (elastic#75486)
  [Maps] add drilldown support map embeddable (elastic#75598)
  [Enterprise Search] Request handler refactors/enhancements + update existing routes to use shared handler (elastic#76106)
  [Resolver] model `location.search` in redux (elastic#76140)
  [APM] Prevent imports of public in server code (elastic#75979)
  fix eslint issue
  skip flaky suite (elastic#76223)
  [APM] Transaction duration anomaly alerting integration (elastic#75719)
  [Transforms] Avoid using "Are you sure" (elastic#75932)
  [Security Solution][Exceptions] - Fix bug of alerts not updating after closure from exceptions modal (elastic#76145)
  [plugin-helpers] improve 3rd party KP plugin support (elastic#75019)
  [docs/getting-started] link to yarn v1 specifically (elastic#76169)
  [Security_Solution][Resolver] Resolver loading and error state (elastic#75600)
  Fixes App Search documentation links (elastic#76133)
  Fix alerts unable to create / update when the name has trailing whitepace(s) (elastic#76079)
  [Resolver] Fix useSelector usage (elastic#76129)
  [Enterprise Search] Migrate util and components from ent-search (elastic#76051)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/data_tier_allocation/node_allocation.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/services/policies/types.ts
#	x-pack/plugins/index_lifecycle_management/public/application/services/policies/warm_phase.ts
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 31, 2020
@kibanamachine
Copy link
Contributor

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.

michaelolo24 added a commit that referenced this pull request Sep 1, 2020
…75600) (#76192)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Resolver Security Solution Resolver feature release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Data Visibility Team managing the endpoint resolver v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants