-
Notifications
You must be signed in to change notification settings - Fork 887
fix: update tests for useClipboard to minimize risks of flakes #13250
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
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.
still pretty inscrutable to me, but maybe slightly more scrutable than before? 😵💫
site/src/hooks/useClipboard.test.tsx
Outdated
console.error = (errorValue, ...rest) => { | ||
const canIgnore = | ||
errorValue instanceof Error && | ||
errorValue.message === COPY_FAILED_MESSAGE; | ||
|
||
if (!canIgnore) { | ||
originalConsoleError(errorValue, ...rest); | ||
} | ||
}; |
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 there a reason we can't use spyOn
or something? feels like jest could be helping with 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.
Oh, yeah, good call – just wasn't thinking straight when I wrote that. Just fixed things
@aslilac Do you think there's anything else I could do to make the code more scrutable? I wish the tests were a bit more straightforward, but I don't know how much of that is the nature of what's being tested, vs my own abilities to write easy-to-follow code |
I think it's a pretty hard spot to scratch, and it might not be worth pouring even more time into as long as this fixes the flakes. 😅 |
Closes #13240
Changes made
describe.each
to improve test isolation between HTTPS and HTTP-only test cases