Restore prettyDOM logging for Cypress #1018
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 e716357:
|
|
I don't have any context why this code is required but I'm all for removing code that's only relevant to another testing library. @testing-library/cypress Is this change ok for you? @istateside Seems like we have some uncovered lines now. Either we need new tests for these or we should remove them. |
|
I'll try to work on getting the coverage increased today 👍 |
|
Looks good to me 👍 |
istateside
force-pushed
the
kf/restore-pretty-dom-for-cypress
branch
from
90f331f to
e716357
Compare
| container, | ||
| )}` | ||
| : null, | ||
| `Ignored nodes: comments, <script />, <style />\n${prettifiedDOM}`, |
There was a problem hiding this comment.
Removed the handling of prettifiedDOM.length === 0 from the default getElementError function. From what I could tell, the only case where that was true was when the function was called from within Cypress. Since we've removed the Cypress special-casing in this PR, this branch no longer seemed useful.
Codecov Report
@@ Coverage Diff @@
## main #1018 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 25 25
Lines 927 916 -11
Branches 294 286 -8
=========================================
- Hits 927 916 -11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@all-contributors please add @istateside for code and tests |
1 similar comment
|
@all-contributors please add @istateside for code and tests |
|
I've put up a pull request to add @istateside! 🎉 |
|
Maintainers: I apologize for the admin-merge here. For some reason I thought this was the cypress repo and I know that not as many people are working in that one so I thought it would be appropriate. I'll try to remember to just approve the PR in the future. I hope this didn't cause any issues. |
|
🎉 This PR is included in version 8.3.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
What:
This PR brings back the
prettyDOMlogging when the library is used within a cypress test (including from within cypress-testing-library)Why:
testing-library/cypress-testing-library#172
Back in 2018, a decision was made to remove prettyDOM logging when used within a Cypress test. This was done because Cypress's default subject is the full document, and pretty-printing the
outerHTMLof the entire document would occasionally cause memory errors.This change was made here- https://github.com/testing-library/dom-testing-library/pull/336/files#diff-2a50dc385b740f1ed6423e95977cc8dcccced5b7d8088e1dd7a02d062872cf1eR18-R19
Discussion around the decision is here - #44 (comment)
Since then, a number of changes have been made to the prettyDOM function that removes (lessens?) this risk. Most importantly, there is now a default max length on these error messages of 7000 characters.
Users also have the ability to opt into defining their own messages printed from the library by passing a
getElementError(message, container)function when performing the initial configuration for the cypress-testing-library.How:
Via extensive use of
DELkeyI've removed the Cypress-specific checks that were implemented in prettyDOM, and removed the test checking for this behavior.
Checklist:
docs site N/A