-
Notifications
You must be signed in to change notification settings - Fork 85
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
Export DOMException; remove custom assertions; etc. #174
Conversation
test/dom/attr.test.js
Outdated
"<xml xmlns:a='a' xmlns:b='b' xmlns='e'><child/></xml>", | ||
'text/xml' | ||
).documentElement | ||
root.setAttributeNS('a', 'a:a', '1') | ||
assert(root.attributes.length, 4) | ||
expect(root.attributes.length).toEqual(4) | ||
//not standart |
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.
//not standart | |
//not standard |
const root = new DOMParser().parseFromString('<xml/>', 'text/xml') | ||
.documentElement |
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 would have reformatted it ...
like this
const root = new DOMParser().parseFromString('<xml/>', 'text/xml') | |
.documentElement | |
const root = new DOMParser() | |
.parseFromString('<xml/>', 'text/xml') | |
.documentElement |
but Prettier does not give us the option: prettier/prettier#7884 ... argh!
Thanks @karfau for your work on this. You should see that I have updated the title, please feel free to change it again if needed. I will likely push some updates and leave a few more suggestions. |
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.
LGTM if we can add DOMException
to the documentation. I did push some updates which are mostly "nits". And a gentle reminder to merge this in a squash commit. Thanks again!
test/dom/attr.test.js
Outdated
//not standart | ||
// root.firstChild.setAttributeNode(root.attributes[0]); | ||
// assert(root.attributes.length, 0); | ||
// expect(root.attributes).toHaveLength(0); |
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 it would be really awesome if we could understand why these commented-out lines are here and if there is anything we can do to resolve them. These commented-out lines seem to have come from this commit back in 2012: c9f94a8
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.
My first guess:
It's a test for something that is not specified in WC3 standard.
If it passes we should just activate and document it (in a different PR), since people might rely on it.
If it doesn't pass we should get rid of the comment :)
Will check it.
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.
Even though adding those lines leaves a passing test, it leaves the DOM in a broken state: the serialization of rootNode
or rootNode.firstChild
fails with Attribute in use
.
Obviously it was not transferred to the new parent/owner correctly.
So I think we should just drop those lines. I don't think they hint to anything that is worth having a test for right now.
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 would kept the comment, ideally with as much historical context as we know, until we get a chance to test the functionality. As I said in #72 (comment) this looks like untested functionality.
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.
Even though adding those lines leaves a passing test, it leaves the DOM in a broken state
@karfau I missed that comment, guess you are right to remove it then. Hope we get a chance to test it someday:)
@brodybits We seem to using a mix of Should I also fix the other places of |
Yes. I think I would prefer fixing them in a separate PR. |
I think I'm done with my changes in this PR. Can you have another look at the new changes? |
and the related vows test suite that was no longer run. (see #72 (comment))
This way the test output is no longer cluttered with all those warnings.
I also took care of converting
var
toconst
andlet
in the test files I touched.