◐ Shell
reader mode source ↗
Skip to content

bpo-40129: Add fake number classes in test.support.#19262

Closed
serhiy-storchaka wants to merge 1 commit into
python:masterfrom
serhiy-storchaka:test-support-fake-numbers
Closed

bpo-40129: Add fake number classes in test.support.#19262
serhiy-storchaka wants to merge 1 commit into
python:masterfrom
serhiy-storchaka:test-support-fake-numbers

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Mar 31, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka added the tests Tests in the Lib/test dir label Mar 31, 2020
@serhiy-storchaka serhiy-storchaka force-pushed the test-support-fake-numbers branch from 7ccc57f to cac62dd Compare March 31, 2020 21:24
pganssle
pganssle reviewed Apr 1, 2020
View reviewed changes

@pganssle pganssle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide 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.

@skrah

skrah commented Apr 1, 2020

Copy link
Copy Markdown
Contributor

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.

@serhiy-storchaka

serhiy-storchaka commented Apr 4, 2020

Copy link
Copy Markdown
Member Author

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?

@serhiy-storchaka serhiy-storchaka deleted the test-support-fake-numbers branch April 11, 2020 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pganssle pganssle pganssle left review comments

@mdickinson mdickinson Awaiting requested review from mdickinson

@abalkin abalkin Awaiting requested review from abalkin

@rhettinger rhettinger Awaiting requested review from rhettinger

+1 more reviewer

@skrah skrah

Assignees

No one assigned

Labels

awaiting core review tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@serhiy-storchaka @skrah @mdickinson @pganssle @the-knights-who-say-ni @bedevere-bot