bpo-43478: Restrict use of Mock objects as specs

[msuozzo]

https://bugs.python.org/issue43478

[gpshead]

Can you use blurb or https://blurb-it.herokuapp.com/ to add a news entry?

[msuozzo]

As noted in one of the commits, this uncovers a bug in an existing test related to https://bugs.python.org/issue37251. Not sure whether to file another bug but the fix should probably be backported as far as 3.8 if, indeed, the tested behavior was intended.

[miss-islington]

Thanks @msuozzo for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

[miss-islington]

Sorry, @msuozzo and @gpshead, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker dccdc500f9b5dab0a20407ae0178d393796a8828 3.9

[miss-islington]

Sorry @msuozzo and @gpshead, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker dccdc500f9b5dab0a20407ae0178d393796a8828 3.8

[gpshead]

Lets not rush the 3.9 and 3.8 backport PRs anyways (manual intervention required anyways); let this ship in 3.10b1 (May) and see if anyone pipes up.

[cjw296]

Honestly not a fan of merging a PR which adds a skip around a test. @voidspace / @lisroach / @tirkarthi : thoughts?

[gpshead]

In general, I agree, a skip or deletion of an existing test always requires extra attention. That test was apparently not-quite-right, this PR exposed that problem which had been hidden. https://bugs.python.org/issue37251 has been reopened which seems like a better place to untangle that than in this merged PR comment thread.

[cjw296]

Right, but I would have preferred to see that bug resolved before this PR was merged. Are there other examples of skipped tests like this in cpython or has this merge just set a bad precedent?

[pablogsal]

There was an error on the travis CI doc build:

$ make check html SPHINXOPTS="-q -W -j4"
python3 tools/rstlint.py -i tools -i ./venv -i README.rst
No problems found.
python3 tools/rstlint.py ../Misc/NEWS.d/next/
[2] ../Misc/NEWS.d/next/Library/2021-04-10-03-30-36.[bpo-43478](https://bugs.python.org/issue43478).iZcBTq.rst:1: default role used
1 problem with severity 2 found.
Makefile:204: recipe for target 'check' failed
make: *** [check] Error 1

Is likely related to the NEWS entry. Apparently is not valid resT or something similar.

[pablogsal]

There was an error on the travis CI doc build:

$ make check html SPHINXOPTS="-q -W -j4"
python3 tools/rstlint.py -i tools -i ./venv -i README.rst
No problems found.
python3 tools/rstlint.py ../Misc/NEWS.d/next/
[2] ../Misc/NEWS.d/next/Library/2021-04-10-03-30-36.[bpo-43478](https://bugs.python.org/issue43478).iZcBTq.rst:1: default role used
1 problem with severity 2 found.
Makefile:204: recipe for target 'check' failed
make: *** [check] Error 1

Is likely related to the NEWS entry. Apparently is not valid resT or something similar.

Fixed by #25335

[msuozzo]

Right, but I would have preferred to see that bug resolved before this PR was merged. Are there other examples of skipped tests like this in cpython or has this merge just set a bad precedent?

I found a few disabled tests in the repo so I figured it wasn't a problem. Ex. https://github.com/python/cpython/blob/master/Lib/test/test_pydoc.py#L1013

Still, I think this is the preferable way to order the changes: A test that provides a false impression of some functionality should not remain unchanged once it's discovered. At the same time, a broken test should not block progress elsewhere in the codebase.

Fixing forward in this PR or in another one would have required more/longer discussion since it would represent a non-trivial change in behavior. This discussion would be fundamentally unrelated to this PR.

As such, I think temporarily annotating the test with its functional status (i.e. that it's broken) and reopening the issue causes the least obstruction while still moving towards addressing the issue.

[pablogsal]

As such, I think temporarily annotating the test with its functional status (i.e. that it's broken) and reopening the issue causes the least obstruction while still moving towards addressing the issue.

Normally this is done in a separate PR with an explanation of what is broken and why we are ignoring it. In this case, unfortunately, is not clear what's broken as the comment redirects to the entire discussion. Also, not only the test is ignored but is also modified from

    def test_create_autospec_awaitable_class(self):
        awaitable_mock = create_autospec(spec=AwaitableClass())
        self.assertIsInstance(create_autospec(awaitable_mock), AsyncMock)

to

    @unittest.skip('Broken test from https://bugs.python.org/issue37251')
    def test_create_autospec_awaitable_class(self):
        self.assertIsInstance(create_autospec(AwaitableClass), AsyncMock)

At the very least, I would suggest documenting in the test exactly what is broken, but ideally, it should be either fixed or removed. Having code that never runs is not a good long term idea.

[cjw296]

I found a few disabled tests in the repo so I figured it wasn't a problem. Ex. https://github.com/python/cpython/blob/master/Lib/test/test_pydoc.py#L1013
...
As such, I think temporarily annotating the test with its functional status (i.e. that it's broken) and reopening the issue causes the least obstruction while still moving towards addressing the issue.

I'm afraid the example you cite is exactly why this ends up being a bad idea: I've highlighted your "temporarily annotating" above as the three skips in test_pydoc.py were introduced between 2014 and 2016; once a skip is committed, it's unlikely to be revisited, even with the best will in the world.

In this case, it's slightly worse as the backport of mock has code coverage checks, which means this then means you've shunted work onto me that I'll have to figure out before I can do a backport release.

[msuozzo]

Normally this is done in a separate PR with an explanation of what is broken and why we are ignoring it. In this case, unfortunately, is not clear what's broken as the comment redirects to the entire discussion. Also, not only the test is ignored but is also modified from...

@pablogsal yeah I think you're right. My intention was to revise the test to perform the check that was actually intended so that it might be easier to revise when addressed. Still, I do think that sort of change could have been left for a separate PR.

Having code that never runs is not a good long term idea.

Definitely.

I'm afraid the example you cite is exactly why this ends up being a bad idea: I've highlighted your "temporarily annotating" above as the three skips in test_pydoc.py were introduced between 2014 and 2016; once a skip is committed, it's unlikely to be revisited, even with the best will in the world.

@cjw296 I think this is a different situation. Realistically, there just needs to be a decision about whether create_autospec behaves as indicated in the test (i.e. requires a fix) or doesn't (i.e. the test can be removed).

In this case, it's slightly worse as the backport of mock has code coverage checks, which means this then means you've shunted work onto me that I'll have to figure out before I can do a backport release.

I'm not familiar with the code coverage checks to which you're referring. Are there coverage checks on the test code itself that skipping will cause to regress? If not, the test in question actually covered no code since it's only assertion was itself mocked out so I wouldn't expect this to be an issue.

[msuozzo]

#25347 should fix the create_autospec

[Mariatta]

Does this still need backport to 3.8 and 3.9? If not, please remove the labels.

[cjw296]

So, 1.5yrs later and this skipped test is still here, and now going to cause coverage problems in the backport :-(
@gpshead - since you merged this, please can you fix the problem in this skipped test?

[msuozzo]

Looks like #25347 never got a review from @lisroach

[cjw296]

@msuozzo - as the author of the problematic skip, you're also welcome to fix it. In the absence of anything, your earlier statement: "Realistically, there just needs to be a decision about whether create_autospec behaves as indicated in the test (i.e. requires a fix) or doesn't (i.e. the test can be removed)." - by skipping that test, you've basically implicitly chosen the second option, so if this ends up blocking me, I'll just remove it.

[gpshead]

Removing the test or leaving the skip in place are both fine. If you can find anyone willing to review #25347 it'll fix it or re-add the test. I suspect that PR is "good" but there are some open questions on it and it isn't immediately obvious to me if they are relevant.