-
-
Notifications
You must be signed in to change notification settings - Fork 216
Add test cases for other valid label separators in IDN hostnames #760
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
Add test cases for other valid label separators in IDN hostnames #760
Conversation
Hi, @Julian. Could you please take a look when you have a free time? |
@@ -336,6 +336,36 @@ | |||
"description": "single dot", | |||
"data": ".", | |||
"valid": false | |||
}, | |||
{ | |||
"description": "single ideographic full stop (RFC 3490#3.1)", |
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.
We have a field for referencing RFC sections in test cases now even though we haven't used it a lot yet -- can you have a look at that and complain if it's undocumented? You'll at least find it in the test case schema
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.
Sure, will take a look at that
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 looked through the documentation - there is not much information about spec links. But if you know the JSON scheme you can find out how it should be defined by reading test-schema.json
file. I think it would be much easier to understand how to use links if the README had a dedicated section for links with examples.
Also, according to test-schema.json
schema the specification
block is allowed only at the top level - it is impossible to add a spec link for a particular test (maybe the schema should be updated to allow spec links for the top level and for a separate test). Should I create a ticket for that?
Also, I have noticed that a link used for RFC (https://www.rfc-editor.org/rfc/{spec}.txt#{section}) does not support anchors. E.g. https://www.rfc-editor.org/rfc/rfc3490.txt - there is no way to put an anchor for a certain section. Probably, an HTML version should be used instead of TXT (e.g. https://www.rfc-editor.org/rfc/rfc3490.html#section-3.1).
And also, it looks like bin/annotate-specification-links
needs some adjustments to support RFC links (if I have added it correctly in the first place).
Please, let me know if I can help with something (at least I can create tickets for some points from above if you agree with them).
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 much easier to understand how to use links if the README had a dedicated section for links with examples.
I definitely agree! I'm not sure if we discussed adding this when we added the capability and just forgot or something -- but yes agree for sure.
And I agree with all of the rest of what you mention as well, so all quite good feedback (and tickets + PRs definitely would be appreciated!)
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.
In this case, I think I could start with fixing the script that adds annotations with spec links (before merging this PR)
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 have finally realised why the specification can be defined only on a test case level. It all started to make sense now) I have moved separator-related tests into a separate test case
3e24090
to
e68286a
Compare
e68286a
to
4fa572d
Compare
Hi, @Julian. Just wanted to check if you had a chance to take a look at the PR after recent changes. Please let me know if something needs to be changed or maybe somebody else should also take a look at the PR |
I haven't looked yet but I'll get to it in the next couple of days if no one gets to it first! |
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.
OK this is perfect, thanks, the citation made this quite easy to double check. LGTM!
I'm applying these tests against my implementation, and I'm wondering -- should the test "dot as label separator" also be applied to hostname.json? |
Sounds likely yeah but needs double checking in that rfc |
I think this case is already covered by this test |
Maybe instead the existing tests for |
There's something different about "a.b" specifically, because my implementation passed all previous tests, but format: idn-hostname failed on this data instance... and now I see that "a.b" fails on hostname as well:
|
Hm... the only reason for the failure that comes into my mind is domain label length. But I couldn't find any rfc that would restrict label length to
|
Just out of curiosity: @karenetheridge could you please check if it still fails once each label has 2 characters? E.g. |
fails: "x.y", "a.xy", "a.b.c" passes: "x.ca", "a.ca", "a.b.ca", "ab.cd" The |
Aha!
Setting this option looks like it will fix all my discrepancies. (edit: I had to set 'domain_allow_single_label' as well, to remove the remainder of my 'todo' items for this format.) https://metacpan.org/pod/Data::Validate::Domain#is_domain($domain,-\%options) |
@karenetheridge @Julian what do you think about creating a separate test case for note sure for now which RFC can be used as a reference for idn-hostname |
sounds good to me! |
According to RFC 3490 Section 3.1, the following characters should be recognized as a dot in IDN hostname:
This PR adds test cases for the remaining characters.
Please, let me know if you have any questions.
Continues #759