Skip to content

bpo-35022: Add __fspath__ support to unittest.mock.MagicMock#9960

Merged
vstinner merged 5 commits into
python:masterfrom
maxbelanger:add-fspath-magicmock
Oct 25, 2018
Merged

bpo-35022: Add __fspath__ support to unittest.mock.MagicMock#9960
vstinner merged 5 commits into
python:masterfrom
maxbelanger:add-fspath-magicmock

Conversation

@maxbelanger

@maxbelanger maxbelanger commented Oct 19, 2018

Copy link
Copy Markdown
Contributor

This is a naive implementation of what has been working for us internally.

https://bugs.python.org/issue35022

The `MagicMock` class supports many magic methods, but not `__fspath__`. To ease
testing with modules such as `os.path`, this function is now supported by default.
@vstinner

Copy link
Copy Markdown
Member

@mariocj89: Would you mind to review this change?

Comment thread Doc/library/unittest.mock.rst
Comment thread Lib/unittest/test/testmock/testmagicmethods.py Outdated

@vstinner vstinner 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.

LGTM. But I would prefer to have a second review, @mariocj89?

@mariocj89 mariocj89 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Feels a little "weird" but I guess there is no better way, I've added some minor suggestions in case you find them useful @vstinner

Comment thread Lib/unittest/mock.py Outdated
'__hash__': lambda self: object.__hash__(self),
'__str__': lambda self: object.__str__(self),
'__sizeof__': lambda self: object.__sizeof__(self),
'__fspath__': lambda self: "%s/%s" % (type(self).__name__, id(self))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will elide all naming information. See:

>>> os.fspath(m.absolute)
'MagicMock/140083495922656'
>>> str(m.absolute)
"<MagicMock name='mock.absolute' id='140083495922656'>"

Or when a mock is given a name. The id is unique per attribute and probably quite unuseful for the user when debugging issues.

Have you considered:
lambda self: f"{self._extract_mock_name()}/{id(self)}"

It would look nicer in some scenarios like:

>>> os.fspath(mock.MagicMock(**{"__fspath__.return_value": "abc"}).absolute)
'mock.absolute/140476946856592'

But maybe weird for some others like:

>>> os.fspath(mock.MagicMock() / 'other' / 'path')
'mock.__truediv__().__truediv__()/140476946879824'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree that naming information should be preserved, what would you think of:

lambda self: f"{type(self).__name__}/{self._extract_mock_name()}/{id(self)}"

The __truediv__ case is a bit strange but it is correct, though.

Comment thread Lib/unittest/mock.py Outdated
@vstinner

Copy link
Copy Markdown
Member

@mariocj89: does it look better to you?

@mariocj89

Copy link
Copy Markdown
Contributor

Sure! I think it will be easier to debug. :)

@vstinner

Copy link
Copy Markdown
Member

Sure! I think it will be easier to debug. :)

I mean: can you please review the updated PR? Like approving it if you consider that it now looks good to you?

@mariocj89 mariocj89 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Much better indeed! :)

@vstinner
vstinner merged commit 6c83d9f into python:master Oct 25, 2018
@maxbelanger
maxbelanger deleted the add-fspath-magicmock branch October 26, 2018 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants