-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
bump @testing-library #78270
bump @testing-library #78270
Conversation
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.
Notes below
@@ -29,4 +29,4 @@ import '@testing-library/jest-dom'; | |||
import { configure } from '@testing-library/react/pure'; | |||
|
|||
// instead of default 'data-testid', use kibana's 'data-test-subj' | |||
configure({ testIdAttribute: 'data-test-subj' }); | |||
configure({ testIdAttribute: 'data-test-subj', asyncUtilTimeout: 4500 }); |
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.
before 4500 was a default. now it is 1000. Forcing 4500 back as some tests were failing
|
||
// Wrap the hook with a provider so it can useApmPluginContext | ||
const wrapper = MockApmPluginContextWrapper; | ||
|
||
describe('useFetcher', () => { | ||
describe('when resolving after 500ms', () => { | ||
let hook: ReturnType<typeof renderHook>; |
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.
Something around types of renderHook
has changed that it is now more strict and doesn't allow use hook
with this inferred type
@@ -31,7 +35,16 @@ const renderUseMetricsExplorerDataHook = () => { | |||
return <KibanaContextProvider services={services}>{children}</KibanaContextProvider>; | |||
}; | |||
return renderHook( | |||
(props) => |
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.
Otherwise it says that props.options
and others are missing. I didn't find a way how to nicely infer it from useMetricsExplorerData
, so just copied types
.find(`[data-test-subj="description-action"] [data-test-subj="property-actions-quote"]`) | ||
.first() | ||
.simulate('click'); | ||
wrapper |
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.
Internal implementation of waitFor
significantly changed between versions. Not sure why exactly, but I had to move this check inside waitFor
to make test pass
@@ -76,7 +76,8 @@ describe('Transform: <DefinePivotForm />', () => { | |||
|
|||
// Act | |||
// Assert | |||
expect(getByLabelText('Index pattern')).toBeInTheDocument(); |
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.
Here it threw error that Index pattern
label is labelling span
, which is incorrect.
I fixed the test, but this component should be reviewed from labels/a11y perspective
This was the error here:
TestingLibraryElementError: Found a label with the text of: Index pattern, however the element associated with this label (<span />) is non-labellable [https://html.spec.whatwg.org/multipage/forms.html#category-label]. If you really need to label a <span />, you can use aria-label or aria-labelledby instead.
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.
Thanks for flagging this up @Dosant . Yes we have a <span>
inside an <EuiFormRow>
on this page. Do you know what would be better from the a11y perspective? Should we replace the <span>
with EuiText
? This form row is displaying a non-editable piece of text (here 'gallery-*' is the span content):
@@ -80,7 +80,7 @@ test('If not enough license, button is disabled', () => { | |||
// check that all factories are displayed to pick | |||
expect(screen.getAllByTestId(new RegExp(TEST_SUBJ_ACTION_FACTORY_ITEM))).toHaveLength(2); | |||
|
|||
expect(screen.getByText(/Go to URL/i)).toBeDisabled(); |
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.
There is button with a lot of nested content inside. It stopped returning a button, but returns a nested p
instead.
@@ -77,7 +78,8 @@ test('Allows to manage drilldowns', async () => { | |||
target: { value: URL }, | |||
}); | |||
|
|||
[createHeading, createButton] = screen.getAllByText(/Create Drilldown/i); |
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.
createButton
is a button with nested content inside. It stopped returning a button, but returns a nested p instead.
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.
Changes on files under operations team code owners LGTM
Pinging @elastic/apm-ui (Team:apm) |
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.
infra
plugin changes LGTM
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.
Thanks for taking the time @Dosant !
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.
😍 Thank you @Dosant (SIEM/Endpoint)
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.
Transforms text edit LGTM.
Do you know what we should be using instead of the <span>
?
@@ -76,7 +76,8 @@ describe('Transform: <DefinePivotForm />', () => { | |||
|
|||
// Act | |||
// Assert | |||
expect(getByLabelText('Index pattern')).toBeInTheDocument(); |
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.
Thanks for flagging this up @Dosant . Yes we have a <span>
inside an <EuiFormRow>
on this page. Do you know what would be better from the a11y perspective? Should we replace the <span>
with EuiText
? This form row is displaying a non-editable piece of text (here 'gallery-*' is the span content):
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
💚 Build SucceededMetrics [docs]async chunks size
page load bundle size
History
To update your PR or re-run it, just comment with: |
Summary
Bumps
@testing-library/*
libsNotable changes:
wait
is deprecated.waitFor
andfind
should be used. docsuser-event
add-on, which is better version of lower levelfireEvent
https://github.com/testing-library/user-eventNote:
@testing-library/react/pure
instead of@testing-library/react
andcleanUp()
manually