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

fix: update tests for useClipboard to minimize risks of flakes #13250

Merged
merged 12 commits into from May 15, 2024

Conversation

Parkreiner
Copy link
Contributor

@Parkreiner Parkreiner commented May 12, 2024

Closes #13240

Changes made

  • Updated all test setup/teardown logic to avoid mishaps with mock timeouts, and also to increase test isolation
  • Updated test approach to use describe.each to improve test isolation between HTTPS and HTTP-only test cases
  • Updated how mock clipboard was defined
  • Added extra test cases for checking what happens when a clipboard write fully, 100% fails

@Parkreiner Parkreiner self-assigned this May 12, 2024
@Parkreiner Parkreiner requested review from a team and aslilac and removed request for a team May 12, 2024 21:11
Copy link
Member

@aslilac aslilac left a 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? 😵‍💫

Comment on lines 160 to 168
console.error = (errorValue, ...rest) => {
const canIgnore =
errorValue instanceof Error &&
errorValue.message === COPY_FAILED_MESSAGE;

if (!canIgnore) {
originalConsoleError(errorValue, ...rest);
}
};
Copy link
Member

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

Copy link
Contributor Author

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

@Parkreiner
Copy link
Contributor Author

@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

@aslilac
Copy link
Member

aslilac commented May 15, 2024

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. 😅

@Parkreiner Parkreiner merged commit 63e0685 into main May 15, 2024
33 checks passed
@Parkreiner Parkreiner deleted the mes/clipboard-flake branch May 15, 2024 20:59
@github-actions github-actions bot locked and limited conversation to collaborators May 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test flake: Tests for useClipboard don't consistently flush state changes
2 participants