-
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
fix: Restore
behavior from v0.1.27
#67
Conversation
Might be considered a breaking change. fixes xmldom#57
Thanks. This would definitely be breaking for anything that does not expect |
I think it's a question of definition. Your decision. |
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.
Approved to be merged after PR #77 - it would be nice for me to validate that the Stryker dashboard is updated with this change.
I do have one non-blocking suggestion.
@@ -13,13 +13,8 @@ wows.describe('html normalizer').addBatch({ | |||
assert(dom+'', '<div xmlns="http://www.w3.org/1999/xhtml"><123e>&<a<br/></div>'); | |||
|
|||
var dom = new DOMParser().parseFromString('<div> © ©</div>','text/html'); |
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.
var dom = new DOMParser().parseFromString('<div> © ©</div>','text/html'); | |
var dom = new DOMParser().parseFromString('<div>© ©</div>>','text/html'); |
(simplify the test string and fix the indentation)
Or do we need to keep the " ©" part to test something? If so, I would kinda favor separate test strings, with separate test assertions, to check the
behavior and to test with " ©".
I do think it would be better to break this test script into smaller test cases, and potentially multiple files, at some point.
TODO: The assertion would have to be updated as well. Unfortunately the GitHub suggested changes feature does not work for multiple lines if some lines were deleted in the middle.
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 agree that the tests are not ideal, that's why I raised #72 to discuss the options.
I would avoid to mix this into other PRs to not loose track.
|
||
|
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 went ahead with the merge, and the change does now show up on the Stryker dashboard. Thanks! |
Might be considered a breaking change.
fixes #57