irc.utils, test: also remove null (\x00) in safe() #2620
Conversation
In `test_irc_utils` suite, rewrote `test_safe` and `test_safe_bytes` to generate a bunch more permutations of invalid characters automatically rather than writing them all out. The horse is definitely beaten *far* past death at this point... but the tests sure are thorough(bred). Also renamed `test_safe_null` -> `test_safe_none` to avoid confusion; `test_safe` and `test_safe_bytes` are the cases that check behavior with null characters/bytes. Python's docs technically do refer to `None` as the "null object" once or twice, but most of the docs (and most of us devs) simply call it `None`.
dgw
added
the
Bugfix
Exirel
left a comment
Exirel
left a comment
There was a problem hiding this comment.
Even tho I don't like when tests have too much logic in them, I draw the line at assert inside for-in bloc. I think @pytest.mark.parametrize is a much better option for these tests.
Using `pytest.mark.parametrize()` instead. Two test cases become 12. Note: Yes, it is possible to have pytest treat each tuple generated by `itertools.permutations()` as a single argument. Doing so yields useless generated test-case names like `test_safe[seq0]`, though. Using three separate test parameters of simple types (`str` or `bytes`) yields more informative names like `test_safe[\n-\r-\x00]`.
SnoopJ
left a comment
SnoopJ
left a comment
There was a problem hiding this comment.
Looks good 🐴
Is it maybe also worth mentioning in the safe() docstring that we strip NUL because it's explicitly disallowed by the spec?
|
Hey, this one could easily go into Sopel 8.0.1 to me. Like a hotfix of something that should be done but isn't. |
Yeah, it is, isn't it? I will tweak the |
Link to RFC explaining why CR, LF, and NUL are prohibited, and annotate the version in which NUL stripping was added.
|
Netlify still does branch previews of our docs (for now… they are still stuck on Python 3.8 with no apparent plans to upgrade) but including a preview screenshot of the updated docs for this function anyway didn't seem like a bad idea.
Note I also fixed the |
Description
Fixes #2566.
In
test_irc_utilssuite, rewrotetest_safeandtest_safe_bytesto generate a bunch more permutations of invalid characters automatically rather than writing them all out. The horse is definitely beaten far past death at this point... but the tests sure are thorough(bred).Also renamed
test_safe_null->test_safe_noneto avoid confusion;test_safeandtest_safe_bytesare the cases that check behavior with null characters/bytes. Python's docs technically do refer toNoneas the "null object" once or twice, but most of the docs (and most of us devs) simply call itNone.Checklist
make qa(runsmake lintandmake test)make lintmyself, and tried out the test runner integration in VS Code / Gitpod. Very slick; made iterating my updated tests much easier without always tabbing back to the terminal.Notes
This could go into 8.0.1, but I don't really want to mess around with hotfixes unless there's a major bug. I'm open to suggestions for improving the maintenance-branch workflow; since
masteris de facto for 8.1.0 dev now, the options are to keep the8.0.xbranch unprotected so I can push cherry-picks to it directly, or make @sopel-irc/rockstars review the same PR twice (once tomaster, once to8.0.x) for full CI coverage. I don't love either of those.