-
Notifications
You must be signed in to change notification settings - Fork 470
fix: improve error message when no window found #1089
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: improve error message when no window found #1089
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 8b89090:
|
The validate is failing since coverage is not 100%. I tried to simulate a test when an HTMLElement is detached but I wasn't able to. test('HTMLElement with no window', () => {
const element = document.createElement('div')
const removedElement = element.ownerDocument.removeChild(element)
expect(() =>
getWindowFromNode(removedElement),
).toThrowErrorMatchingInlineSnapshot()
}) But it's failing saying that |
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 think we can do something like this:
test('htmlElement as node', () => {
const elem = document.createElement('div')
Object.defineProperty(elem, 'ownerDocument', {
get: function get() {
return null;
},
});
expect(() => getWindowFromNode(elem)).toThrowErrorMatchingInlineSnapshot(
`Unable to find the "window" object for the given node. Please file an issue with the code that's causing you to see this error: https://github.com/testing-library/dom-testing-library/issues/new`,
)
})
Good idea, that did the trick :) |
Codecov Report
@@ Coverage Diff @@
## main #1089 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 25 25
Lines 954 956 +2
Branches 314 315 +1
=========================================
+ Hits 954 956 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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'm still wondering if that's even a valid use-case that can happen, but I pushed it :)
Me neither 😅 - but since someone opened an issue, there must be a few of those cases. Now, I hope this message will help them.
…m:MatanBobi/dom-testing-library into feat/improve-error-message-when-no-window
Thanks for the review @eps1lon, anything preventing us from merging 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.
I'll also approve this PR since my review was still pending, just in case I was block this.
Just cant merge from the app. Will do when I'll have access to a desktop |
🎉 This PR is included in version 8.11.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What: Improves error message when window cannot be found
Why: Resolves #863
How: Narrow the
else
fallbackChecklist:
docs site - N/A