-
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
[Security solution][Endpoint] Add unit tests for fleet event filters/trusted apps cards #101034
[Security solution][Endpoint] Add unit tests for fleet event filters/trusted apps cards #101034
Conversation
…_to_the_fleet_endpoint_tab-1157
…core, adding aggs type, validating response, and more
…d small improvements on test -> ts
…_to_the_fleet_endpoint_tab-1157
…_endpoint_tab-1157' of github.com:dasansol92/kibana into feature/olm-add_event_filters_summary_card_to_the_fleet_endpoint_tab-1157
…_to_the_fleet_endpoint_tab-1157
Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt) |
it('should renders correctly when missing some stats', () => { | ||
const summary: Partial<GetExceptionSummaryResponse> = { | ||
windows: 3, | ||
total: 3, |
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.
Do we need a test case to check if the total is missing? is that a possible case?
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.
Yes, that's possible but in the component each item is parsed in the same way, so if some key is missing, a 0 will be displayed instead
@@ -14,11 +14,11 @@ export const StyledEuiFlexGridGroup = styled(EuiFlexGroup)` | |||
`; | |||
|
|||
export const StyledEuiFlexGridItem = styled(EuiFlexItem)<{ | |||
gridArea: string; | |||
alignItems?: string; | |||
gridarea: string; |
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.
is this change and the following intended? (removing the camel case)
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.
Yes! This is to remove a react warning for non recognized attributes:
Warning: React does not recognize the `alignItems` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `alignitems` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
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.
🚀 LGTM. I have only a bunch of nitpicks.
@@ -14,11 +14,11 @@ export const StyledEuiFlexGridGroup = styled(EuiFlexGroup)` | |||
`; | |||
|
|||
export const StyledEuiFlexGridItem = styled(EuiFlexItem)<{ | |||
gridArea: string; | |||
alignItems?: string; | |||
gridarea: string; |
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.
Out of curiosity? Why was a change to flat case needed?
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.
To avoid this react warning:
Warning: React does not recognize the `alignItems` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `alignitems` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
const fetchStats = async () => { | ||
try { | ||
const response = await trustedAppsApi.getTrustedAppsSummary(); | ||
setStats(response); | ||
if (isMounted) setStats(response); |
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.
nitpick: I feel it's more readable with {}
around a single statement.
}; | ||
}); | ||
}); | ||
it('should renders correctly', async () => { |
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.
nitpick: should render...
expect(component.getByText('Trusted Applications')).not.toBeNull(); | ||
expect(component.getByText('Manage trusted applications')).not.toBeNull(); | ||
}); | ||
it('should renders an error when api call fails', async () => { |
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.
nitpick: should render...
afterEach(() => { | ||
TrustedAppsHttpServiceMock.mockReset(); | ||
}); | ||
beforeEach(() => { |
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.
nitpick: semantically it makes sense to keep all before..
methods before after...
methods. So beforeAll
followed by beforeEach
and then afterEach
at the end.
expect(component.getByText('Trusted Applications')).not.toBeNull(); | ||
expect(component.getByText('Manage trusted applications')).not.toBeNull(); | ||
}); | ||
it('should renders an error when api call fails', async () => { |
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.
nitpick: I think it's okay to have test descriptions with even more context. This one renders an error toast and should say something like should render an error toast when...
. That way one doesn't have to read the code to know what this test is verifying.
} | ||
return component; | ||
}; | ||
afterEach(() => { |
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.
nitpick: semantically it makes sense to keep all before...
methods before after...
methods. So beforeAll
followed by beforeEach
and then afterEach
at the end.
}; | ||
}); | ||
}); | ||
it('should renders correctly', async () => { |
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.
nitpick: ...render...
}); | ||
return component; | ||
}; | ||
it('should renders correctly', () => { |
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.
nitpick: ...render...
expect(component.getByText('Event Filters')).not.toBeNull(); | ||
expect(component.getByText('Manage event filters')).not.toBeNull(); | ||
}); | ||
it('should renders an error when api call fails', async () => { |
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.
nitpick: should render an error toast when api call fails
💚 Build SucceededMetrics [docs]Async chunks
Unknown metric groupsReferences to deprecated APIs
History
To update your PR or re-run it, just comment with: |
…trusted apps cards (elastic#101034) * Adds new unit tests for fleet card components * Fixes some warnings on ui * Adds some syntax and readibility nits comming from pr comments Co-authored-by: Kibana Machine <[email protected]>
…sens/kibana into reporting/new-png-pdf-report-type * 'reporting/new-png-pdf-report-type' of github.com:jloleysens/kibana: (46 commits) [Security Solution] Add Ransomware canary advanced policy option (elastic#101068) [Exploratory view] Core web vitals (elastic#100320) [Security solution][Endpoint] Add unit tests for fleet event filters/trusted apps cards (elastic#101034) [Lens] Use a setter function for the dimension panel (elastic#101123) [Index Patterns] Fix return saved index pattern object (elastic#101051) [CI] For PRs, build TS refs before public api docs check (elastic#100791) [Maps] fix line and polygon label regression (elastic#101085) Migrate CCR to new ES JS client. (elastic#100131) [Canvas] Switch Canvas to use React Router (elastic#100579) [Expressions] Use table column ID instead of name when set (elastic#99724) [DOCS] Updates docs landing page (elastic#100749) [DOCS] Corrects typo in step 3 (elastic#101079) [DOCS] Updates runtime example in Discover (elastic#100926) Migrate kibana.autocomplete config to data plugin (elastic#100586) [Uptime] New width/delay definition for waterfall sidebar item tooltip (elastic#100147) [FTR] Use importExport for saved_object/basic archive (elastic#100244) [Fleet] Better input for multi text input in agent policy builder (elastic#101020) [CI] Buildkite support with Baseline pipeline (elastic#100492) [Reporting/Telemetry] Do not send telemetry if we are in screenshot mode (elastic#100388) Create API keys with metadata (elastic#100682) ...
* master: (68 commits) Unskip advanced settings a11y test (elastic#100558) [App Search] Crawler Landing Page (elastic#100822) [DOCS] Clarify when to use kbn clean (elastic#101155) change label behavior (elastic#100991) skip flaky suite (elastic#101126) Fix cases plugin ownership (elastic#101073) [Home] Adding file upload to add data page (elastic#100863) [ML] Functional tests - reenable categorization tests (elastic#101137) [DOCS] Adds server.uuid to settings docs (elastic#101121) Fix newsfeed unread notifications always on when reloading Kibana (elastic#100357) [Lens] Time shift metrics (elastic#98781) [Deprecations service] make `correctiveActions.manualSteps` required (elastic#100997) Add "Risk Matrix" section to the PR template (elastic#100649) [Maps] spatially filter by all geo fields (elastic#100735) [Security Solution] Add Ransomware canary advanced policy option (elastic#101068) [Exploratory view] Core web vitals (elastic#100320) [Security solution][Endpoint] Add unit tests for fleet event filters/trusted apps cards (elastic#101034) [Lens] Use a setter function for the dimension panel (elastic#101123) [Index Patterns] Fix return saved index pattern object (elastic#101051) [CI] For PRs, build TS refs before public api docs check (elastic#100791) ...
…trusted apps cards (#101034) (#101151) * Adds new unit tests for fleet card components * Fixes some warnings on ui * Adds some syntax and readibility nits comming from pr comments Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
Adds new unit tests for new components from this other PR
Checklist
For maintainers