-
Notifications
You must be signed in to change notification settings - Fork 466
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: log ignored selectors correctly on error #1141
fix: log ignored selectors correctly on error #1141
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 a203ff0:
|
}) | ||
|
||
afterEach(() => { | ||
configure(originalConfig) |
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.
Ensure tests don't rely on previous tests changing the config in a certain way.
Codecov Report
@@ Coverage Diff @@
## main #1141 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 24
Lines 990 990
Branches 322 322
=========================================
Hits 990 990
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
@@ -1092,6 +1102,19 @@ test('the default value for `ignore` can be configured', () => { | |||
expect(noStyle[1].tagName).toBe('DIV') | |||
}) | |||
|
|||
test('the default value for `ignore` is used in errors', () => { |
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.
New test that proves that overriding defaultIgnore
produces a new error message
@@ -194,14 +194,14 @@ test('when a promise is returned, if that is not resolved within the timeout, th | |||
await sleep(5) | |||
|
|||
expect((await waitForError).message).toMatchInlineSnapshot(` | |||
Timed out in waitFor. | |||
|
|||
Ignored nodes: comments, <script />, <style /> |
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.
Can we keep the original formatting? "comments, scripts, style" looks like they all refer to the same thing while "scripts, style" specifically refers to elements.
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.
Should be fine. Alternative is another .split().map().join()
which is probably too expensive.
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!
@all-contributors add @robin-drexler for code |
@robin-drexler already contributed before to code |
🎉 This PR is included in version 8.17.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What:
default
getElementError
implementation always logged that script and style are being ignored even when the default has been overridden with thedefaultIgnore
option.It now logs the correct selector.
Since the selector could be any css selector, we can't just print
<script />
etc. Instead we just print the selector as is.Checklist:
docs site