bpo-40129: Add fake number classes in test.support. - #19262
bpo-40129: Add fake number classes in test.support.#19262serhiy-storchaka wants to merge 1 commit into
Conversation
Sorry, something went wrong.
pganssle
left a comment
There was a problem hiding this comment.
Thanks for doing this. The idea seems sound and the datetime implementation seems right.
One suggestion: you should add documentation for this. There is documentation for test.support, but even a block comment before Fake would be helpful to give some additional context to future maintainers and devs about why it's there and when and where you should use it.
Sorry, something went wrong.
| __complex__ = Fake._return | ||
|
|
||
| class FakePath(Fake): | ||
| """Simple implementing of the path protocol. |
There was a problem hiding this comment.
| """Simple implementing of the path protocol. | |
| """Simple implementation of the path protocol. |
Sorry, something went wrong.
|
|
||
| class FakePath(Fake): | ||
| """Simple implementing of the path protocol. | ||
| """ |
There was a problem hiding this comment.
Should this be on the previous line? Looks weird to have it sitting on its own following a short one-line docstring like this.
Sorry, something went wrong.
| def f(): return 7 | ||
|
|
||
| values = [INT(9), IDX(9), | ||
| values = [FakeInt(9), FakeIndex(9), |
There was a problem hiding this comment.
I don't quite understand the name, there's nothing really fake about these classes. Also, it's nice to see what the classes do without leaving the file.
Sorry, something went wrong.
There was a problem hiding this comment.
I agree with both points here.
When scanning through the PR on GitHub and seeing FakeInt(10.9), my first thought is that I don't know what that means without guessing, so I want to quickly find the FakeInt definition. That's harder when that definition is in another file. (And yes, if you have a decent editor open and set up properly, the definition should be just a click or keystroke away, but not everyone has that setup available all of the time.)
The problem could potentially be mitigated with a name that more clearly describes what the class is doing (HasDunderIntReturning(3.2)?), but as you can tell I'm having trouble coming up with a name that's going to be immediately unambiguous for people reading the code without also being unwieldy.
Sorry, something went wrong.
|
IMO tests should be primitive and locally understandable. If they get too fancy, it's easy to lose track of what is being tested. So I'm not sure if much is gained here. |
Sorry, something went wrong.
|
The main purpose is to make easier to test that the specified function supports an object that implements __index__, not just int. There are a lot of such tests in different files, and we can add more tests, but it is tiring to reimplement such class in every file. It would also help if all test classes used for this purpose have a standardized and well-recognized name, so you don't even need to search its definition. I used name FakeIndex because we already have FakePath for similar purposes (used in 11 different files), and when it was introduced it looked a good name. We also have more specialized classes like FakeSocket. Do you have to propose better name? MockIndex, IndexLike or just Index? |
Sorry, something went wrong.
https://bugs.python.org/issue40129