classification
Title: Refactor b32{encode,decode} tests
Type: enhancement Stage: patch review
Components: Tests Versions: Python 3.10
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: FFY00, p-ganssle, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2020-09-06 19:15 by FFY00, last changed 2020-11-29 19:29 by FFY00.

Pull Requests
URL Status Linked Edit
PR 22122 open FFY00, 2020-09-06 19:34
Messages (5)
msg376468 - (view) Author: Filipe Laíns (FFY00) * (Python triager) Date: 2020-09-06 19:15
As discussed in https://github.com/python/cpython/pull/20441, these tests could be improved by using the same format of the b32hex{encode,decode} tests.
msg376516 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-09-07 17:01
I think this change is not needed. The current code is shorter and works pretty well.
msg376528 - (view) Author: Filipe Laíns (FFY00) * (Python triager) Date: 2020-09-07 19:37
Yes, it is not needed, but it does provide a fair bit of enhancement. Using subtests makes more clear what failed when something fails.
I am a bit confused, in PR 20441 I first just copied the current b32{encode,decode} tests but was given feedback which resulted in the proposed tests, but now I am being told the opposite, that the tests are better as they currently are.
Anyway, this is not my call, let me know if something changes.
msg376605 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2020-09-08 21:16
I agree with Filipe here — I think the b32encode/b32decode tests were originally written before subtests were available, and this PR has this and other real improvements.

I understand why you'd want to have a policy of "no refactoring for its own sake", but as I argued in the PR 20441 (https://github.com/python/cpython/pull/20441#issuecomment-634773049), it's safer to leave existing tests alone when making changes to the code under test, since there's the possibility that you both introduce an error *and* modify the tests in such a way that doesn't catch the error you introduced. In that case, "refactoring as you go" doesn't really work, and you need a separate PR for improvements like these.

I'm re-opening the ticket for now because I think we should at least discuss this before rejecting it out of hand.


> I am a bit confused, in PR 20441 I first just copied the current b32{encode,decode} tests but was given feedback which resulted in the proposed tests, but now I am being told the opposite, that the tests are better as they currently are.

Sorry about the mixed messages. I think you simply chalk this up to the fact that Serhiy and I apparently disagree about test structure. I reviewed the previous PR and specifically asked for this change, so I think it was a bit rash to close the issue right away (though as someone who has probably prematurely closed his fair share of issues, I should probably not be tossing about stones in the vicinity of my decidedly double-paned domicile).
msg382091 - (view) Author: Filipe Laíns (FFY00) * (Python triager) Date: 2020-11-29 19:29
Friendly ping on this.
History
Date User Action Args
2020-11-29 19:29:54FFY00setmessages: + msg382091
2020-09-08 21:16:43p-gansslesetstatus: closed -> open
versions: + Python 3.10
messages: + msg376605

resolution: rejected ->
stage: resolved -> patch review
2020-09-07 19:37:20FFY00setmessages: + msg376528
2020-09-07 17:01:17serhiy.storchakasetstatus: open -> closed

nosy: + serhiy.storchaka
messages: + msg376516

resolution: rejected
stage: patch review -> resolved
2020-09-06 19:34:10FFY00setkeywords: + patch
stage: patch review
pull_requests: + pull_request21206
2020-09-06 19:15:07FFY00create