test: add capabilities test cases for trailing space handling #2505
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some IRCds include trailing space in capability lists, and this is not a spec violation. Default `.split()` behavior in Python should handle it fine, but it's also good to be explicit that we expect the default behavior, through tests.
It's not much, but it's honest work. I was going to do some type-hint cleanup too, but there are plenty of warnings in the `test/` directory because we haven't been checking it. That sort of work should be done in its own PR(s) rather than as incidental cleanup "because I'm already touching this file".
Exirel
approved these changes
Sep 4, 2023
|
Spec clarification was merged 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some IRCds include trailing space in capability lists, and this is not a spec violation. Default
.split()behavior in Python should handle it fine, but it's also good to be explicit that we expect the default behavior, through tests.Checklist
make qa(runsmake lintandmake test)Success: no issues found in 82 source files1322 passed, 8 xfailed, 1 warning in 70.41s (0:01:10)Notes
Inspired by ircv3/ircv3-specifications#530, which is not yet merged. However, since this is de facto behavior by UnrealIRCd and InspIRCd that isn't likely to change very soon even if the spec amendment is rejected, I think we should just add the tests.
Review requested from @Exirel specifically because the new capability system is his baby, and he's the most likely one to point out any incorrect assumptions I might have had about how to adapt existing test cases for this purpose.