gh-81005: Refactor str tests to reflect that str and unicode are merged in Python 3 #13172
Conversation
(to reflect the type that is now being tested)
(The comment for this mixin class says
# additional tests that only work for
# stringlike objects, i.e. str, UserString
but in the absence of `unicode` the `CommonTest` class is also only
used for these two types now, so no need for the distinction.)
communicate its purpose, and to disambiguate it from the more general `BaseTest` class, which covers behaviours shared with the `bytes` and `bytesarray` types.
(Seems to date back to the first introduction of this code, in 2003!)
non-str types. Back in 2007 there was a change to allow str.join() to convert non-str values automatically, but this is no longer the case.
raising TypeError, so BadSeq1 and the associated check seem to be
redundant. This is the existing check, in test_join():
self.assertRaises(TypeError, '.'.join, ['a', 'b', 3])
(Unless there is some specific difference we're testing by supplying a
custom sequence, BadSeq1, rather than a plain list?)
- Define BadSeq2 closer to its only usage, and give it a more meaningful
name. (Previously this type was referenced from multiple locations but
this is no longer the case.)
|
As this is a test-only change with no external visibility, I don' think it needs a NEWS entry. Should the "skip news" label be applied to this PR? |
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
@asqui Could you resolve the conflicts? |
|
Sure! I'm AFK for the next few days but I'll take a look when I'm back next week. |
|
All commit authors signed the Contributor License Agreement. |
|
Changes to the renamed file merged in cleanly with absolutely no conflicts. (I was surprised given the age of this PR so did some sanity checks and it all looks good.) CLA signed (again; was there a change to the CLA that required everyone to re-sign?) I think this is good to merge now? (Needs the Skip News label.) |
I think that something like /cc @AlexWaygood (to check whether I apply #104570 (comment) correctly) |
Good call @arhadthedev. Changes to our test suite often don't need NEWS entries, as they don't have much impact on end users. But something as significant as renaming a whole test file probably does need a NEWS entry, as it will affect the invocation people will need to use to run the tests. |
arhadthedev
changed the title
|
Added the suggested NEWS entry. (This is my first time using |
|
Hmm looks like I should have used the GH issue number for the bpo-imported issue that this PR is fixing (I mistakenly put the number for this PR itself) I'm AFK again for the next few days so won't be able to fix this until next week. I guess all is needed is to rename the news file to feature the correct number - I don't know of any way to do that through GitHub.com without my laptop. |
|
I've renamed the NEWS file to use the GitHub issue number. |
|
And I've opened python/core-workflow#504 to suggest more validation in blurb. |
|
Thank you for the PR and for your patience! |
|
Yay, merged at last! Thanks for the reviews and help along the way 😄 |
Various cleanup and refactoring of unit tests for string-like types.
See https://bugs.python.org/issue36824 and/or individual commit messages for more details.
I haven't done the "Promote tests from the current
UnicodeTestclass toCommonTest" portion yet, but I'm running out of sprint time so I wanted to submit what I have.I believe these changes are suitable for merge to master, but keen to hear feedback :-)
https://bugs.python.org/issue36824