-
Notifications
You must be signed in to change notification settings - Fork 209
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
chore: migrate use of @testing-library/react-hooks #867
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
after moving to @testing-library/react
i got some type errors so had to update the packages
FYI @testing-library/react-hooks
did not support react 18 and was running on react 17
@@ -339,32 +310,31 @@ describe('useBalance', () => { | |||
} | |||
]) | |||
) | |||
}) |
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.
assertions shouldn't be wrapped in the same act
function as it was previously
15763b5
to
138545f
Compare
138545f
to
15763b5
Compare
@@ -23,7 +23,7 @@ export function RetryableTxnsIncluder(): JSX.Element { | |||
} = useAppState() | |||
|
|||
const fetchAndUpdateDepositStatus = useCallback( | |||
async (depositTxId, depositAssetType) => { | |||
async (depositTxId: string, depositAssetType: AssetType | 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.
@types/react & @types/react-dom v17 for some reason allow it to be of type any
and won't throw an error but v18 will
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.
Nice, this removes the warnings we got during test:ci :)
Small comments were identified.
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!
import { StaticJsonRpcProvider } from '@ethersproject/providers' | ||
import { BigNumber } from 'ethers' | ||
import { SWRConfig } from 'swr' | ||
import { PropsWithChildren } from 'react' | ||
import { MultiCaller } from '@arbitrum/sdk' | ||
import { useBalance } from '../useBalance' | ||
|
||
import { renderHookAsyncUseBalance } from './utils' |
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 would still keep renderHookAsyncUseBalance
co-located in useBalance
file.
My rationale: If there are 5 more tests like useBalance in the future.. we should have their specific renderHookAsync calls in their own files rather than bloating generic utils
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
Summary
it's good practice to update the library to date esp because
@testing-library/react-hooks
fallbacks to react 17 instead of react 18Note:
waitForNextUpdate
andwaitForValueToChange
are removed whenrenderHook
was migrated to@testing-library/react
and so there are more changes in the tests than a simple library migration / updateSteps to test
tests should work and pass as usual