8000 bpo-35252: Remove FIXME from test_functools by lysnikolaou · Pull Request #10551 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-35252: Remove FIXME from 8000 test_functools #10551

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
May 19, 2019
Merged

bpo-35252: Remove FIXME from test_functools #10551

merged 4 commits into from
May 19, 2019

Conversation

lysnikolaou
Copy link
Member
@lysnikolaou lysnikolaou commented Nov 15, 2018 •
edited by bedevere-bot
Loading

The FIXME was stopping execution of a test that would only work fine after PEP560 implementation. Now that the PEP is implemented the FIXME should be deleted.

https://bugs.python.org/issue35252

@lysnikolaou
bpo-35252: Remove FIXME from test_functools that was stopping executi…
23b5ceb 
…on of a test that would only work fine after PEP560 implementation
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Nov 15, 2018
@remilapeyre
Copy link
Contributor

Hi @lysnikolaou, thanks for noticing this!

You can see why the test fail by running ./python.exe -m test test_functools -v once you have compiled Python:

======================================================================
FAIL: test_invalid_registrations (test.test_functools.TestSingleDispatch)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/remi/src/cpython/Lib/test/test_functools.py", line 2301, in test_invalid_registrations
    def _(arg: typing.Iterable[str]):
  File "/Users/remi/src/cpython/Lib/functools.py", line 851, in register
    assert isinstance(cls, type), (
AssertionError: Invalid annotation for 'arg'. typing.Iterable[str] is not a class.

----------------------------------------------------------------------

singledispatch is a facility to define more easily functions whose behavior depends on the type of their parameter (https://docs.python.org/3/library/functools.html#functools.singledispatch).

When using type hints, one can give both a type like def foo(bar: int): pass or a type annotation like def foo(bar: List[float]): pass.

singledispatch do the dispatching based on the type of the element so type annotations cannot be used here.

The test checks that the correct exception TypeError is raised but the correct check for this in the register method is not yet implemented so an AssertionError is raised a few line further, you just need to add it.

@lysnikolaou
Copy link
Member Author

Hi @remilapeyre,

thanks a lot for your help.

I now have a better picture of what the code does, but I'm still not exactly sure what needs to be done here. Would it be best to just turn the assertRaises(TypeError) into an assertRaises(AssertionError) and delete the assertions that follow? That seems like a whole other test, that should maybe live in another function.

Maybe delete the whole block of code?

@lysnikolaou
Copy link
Member Author

@remilapeyre maybe got some time to give me a bit more info about what I need to do?

@remilapeyre
Copy link
Contributor

Hi @lysnikolaou, thanks for the ping. TypeError is the correct exception to be raised, AssertionError is the wrong kind of exception most of the time. Steven D'Aprano made a comprehensive post where he highlights when to use assert that you can read here: https://mail.python.org/pipermail/python-list/2013-November/660568.html.

For the present PR, I think the test and it's the code at https://github.com/python/cpython/blob/master/Lib/functools.py#L833-L849 that needs fixing to raise TypeError.

Does this make sense?

@lysnikolaou
Copy link
Member Author

It does make a lot of sense @remilapeyre. Thanks a bunch for the help. I have now updated my PR.

@remilapeyre
Copy link
Contributor

Thanks @lysnikolaou, I'm currently blocked by https://bugs.python.org/issue36432 but will make a review as soon as I can.

@remilapeyre
Copy link
Contributor

I forgot, this change will need a news entry, you can add one using https://blurb-it.herokuapp.com/

@lysnikolaou
Copy link
Member Author

Thanks @remilapeyre, let me know in case I can do anything to help make it easier for you.

@lysnikolaou
Copy link
Member Author

Pinging for review.

@csabella csabella requested a review from ambv May 17, 2019 11:03
@ambv ambv merged commit d673810 into python:master May 19, 2019
@ambv
Copy link
Contributor
ambv commented May 19, 2019

Thanks! ✨ 🍰 ✨

@lysnikolaou lysnikolaou deleted the bpo35252 branch May 19, 2019 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhettinger rhettinger Awaiting requested review from rhettinger

@ambv ambv Awaiting requested review from ambv

Assignees
No one assigned
Labels
tests Tests in the Lib/test dir
-->
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lysnikolaou @remilapeyre @ambv @the-knights-who-say-ni @bedevere-bot
0