-
Notifications
You must be signed in to change notification settings - Fork 6
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 comment nodes being specified as children in a satisfy spec. #219
Fix comment nodes being specified as children in a satisfy spec. #219
Conversation
Pull Request Test Coverage Report for Build 822
💛 - Coveralls |
Mhh I never thought of using ignore in an object spec context, in that case you can just use expect.it. Can you point me to the problem in unexpected-reaction. The ignore comment was just syntactic sugar for ignoring with expect.it. |
@sunesimonsen I've documented it here: unexpectedjs/unexpected-reaction#5. Let me know what you think :) |
lib/index.js
Outdated
value.children | ||
Array.isArray(value.children) | ||
? convertChildrenToSatisfySpec(value.children, isHtml) | ||
: value.children |
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.
Shouldn't we always convertChildrenToSatisfySpec, what if I'm providing a DOM tree as a single items? Maybe I don't have the full overview 🤔
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 don't believe so - based on my understanding of the rest of the assertions, I believe it is possible for a DOMNodeList object to be supplied as the value of children
and the comparison should still work. If individual DOM nodes are supplied they'd be in an array which would need the conversion :)
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 realize that I should probably have written more tests for ignore, it was sort of an experiment that got shipped to production - as all prototypes :-)
Sorry about that.
It would be great to get @papandreou take a lot at this as well. I'll take a deeper look at it tomorrow. |
This is definitely the one to consider. What this change specifically allows is full DOM nodes to be supplied as the elements of the children array. It runs them through a conversion to satisfy specs which means all the matching done by that function gets picked up (for example the It also maintains the invariant of plain strings resulting in a text based comparison of the nodes which should mean that it is backwards compatible, something I did not intend to undermine. |
… assertion Fixes <!--ignore--> as the top-level to satisfy RHS.
fe9ce65
to
25bd2b4
Compare
I'd like to get |
Model <!--ignore--> as its own type and implement the ignoring via an assertion
Released as unexpected-dom@4.6.4 |
This is a backwards compatible version of the previous PR that ensures that any DOMNode present in the
children
property of an object spec is converted before comparison.Fixes #217.