Skip to content
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

Refine handling of illegal characters in userinfo #70

Merged
merged 1 commit into from Dec 17, 2018

Conversation

Projects
None yet
3 participants
@sideshowbarker
Copy link
Contributor

commented Dec 17, 2018

No description provided.

@coveralls

This comment has been minimized.

Copy link

commented Dec 17, 2018

Coverage Status

Coverage increased (+0.007%) to 87.891% when pulling 6696412 on validator:illegal-chars-in-userinfo-update into 1e647f6 on smola:master.

@smola

This comment has been minimized.

Copy link
Owner

commented Dec 17, 2018

@sideshowbarker Thanks!

Would you mind adding a test for this in ParseIssueTest? I'm about to push a major rewrite of the parser and I'm having a hard time maintaining validation errors.

@sideshowbarker

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

@smola I added a test for this back at 5e68451#diff-d044dce65d9dfb982ad24b58dabeb501. So that test is now already on the master branch at https://github.com/smola/galimatias/blob/master/src/test/resources/data/urltestdata_whatwg.txt#L331 (the last test in that file).

Should I also add it in ParseIssueTest?

Anyway, to be clear: That test passes both before and after this change. In other words, the patch in this PR doesn’t change the behavior of the existing test when run under the test harness here.

However, it can change the behavior of tests run by consuming code. For example, the test suite I use for the HTML checker (validator) has the following test: https://github.com/web-platform-tests/wpt/blob/master/conformance-checkers/html/elements/a/href/userinfo-username-contains-pile-of-poo-novalid.html. When I run that test without the patch in this PR/branch applied, it incorrectly reports the problem character as "?". But when I run that test with the patch applied, it correctly reports the problem character as "💩".

@smola

This comment has been minimized.

Copy link
Owner

commented Dec 17, 2018

Yes, there should be a test in ParseIssueTest that checks the exception message, not just the ParseIssue.

@sideshowbarker sideshowbarker force-pushed the validator:illegal-chars-in-userinfo-update branch from 4dede44 to 6696412 Dec 17, 2018

@sideshowbarker

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

OK, added a test in the amended commit 6696412 and (force-)pushed to the branch

@smola smola merged commit 35b54cc into smola:master Dec 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 87.891%
Details

@sideshowbarker sideshowbarker deleted the validator:illegal-chars-in-userinfo-update branch Dec 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.