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

Add blank verb test to msg-split #20

Open
paravoid opened this issue Apr 16, 2020 · 3 comments
Open

Add blank verb test to msg-split #20

paravoid opened this issue Apr 16, 2020 · 3 comments

Comments

@paravoid
Copy link

This is a bit of a weird one, because it's fundamentally invalid and I'm not sure how you would handle that in the YAML -- empty verb key perhaps, and let it fail somewhere else in the stack?

Example (broken) input: :foo.

This raises a (cryptic) exception in goshuirc 0.4.0's RFC1459Message, so I think it may be important to test for:

goshuirc-irc/girc/ircreactor/envelope.py in from_message(cls, message)
    104             s = s[1:]
    105 
--> 106         verb = s[0].upper()
    107         original_params = s[1:]
    108         params = []

IndexError: list index out of range

Similar in nature to #12, which is for blank tag/source.

@DanielOaks
Copy link
Member

Yeah it definitely makes sense to have a test in here, if nothing else for safety so that server-side parsers also don't throw unexpected exceptions when getting these messages. We could either add a valid key to all the tests, matching how the hostname tests are laid out, or we could just create a new root element so there's tests and failing-tests. Not sure which one's cleaner but definitely makes sense to add this, thanks for the yell mate.

@paravoid
Copy link
Author

Thanks for the quick response!

Going one step further, it could potentially make sense to also test for all kinds of other exceptional conditions of that sort, e.g. verbs that contain symbols/numbers/non-ASCII characters. Basically anything outside 2812's BNF definition of \w+|\d{3}. Perhaps that's a bit too much of a scope creep for these tests, however?

@DanielOaks
Copy link
Member

DanielOaks commented Apr 16, 2020

No worries! Yeah I'd see that as alright, as an FYI I tend to prefer matching real-world behaviour rather than the RFCs, so I'd wanna confirm there's no commands out there right now that include numbers/_. Just to be safe, since those seem like the kind of limits that servers may have ignored over the years~

paravoid added a commit to paravoid/ircstream that referenced this issue May 16, 2020
paravoid added a commit to paravoid/ircstream that referenced this issue May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants