This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: There is an overshadowed and invalid test in testmock.py
Type: behavior Stage: resolved
Components: Tests Versions: Python 3.4, Python 3.5
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: michael.foord Nosy List: BreamoreBoy, berker.peksag, ezio.melotti, michael.foord, vajrasky, xdegaye
Priority: normal Keywords: patch

Created on 2013-09-10 06:55 by vajrasky, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
remove_wrongly_test_in_testmock.patch vajrasky, 2013-09-10 06:55 review
fix_overshadowed_test_in_testmock.patch vajrasky, 2013-09-14 09:05 review
Messages (11)
msg197424 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-09-10 06:55
In Lib/unittest/test/testmock/testmock.py, there are two test_attribute_deletion. One of them is overshadowed (not executed) by the other.

    def test_attribute_deletion(self):
        # this behaviour isn't *useful*, but at least it's now tested...
        for Klass in Mock, MagicMock, NonCallableMagicMock, NonCallableMock:
            m = Klass()
            original = m.foo
            m.foo = 3
            del m.foo
            self.assertEqual(m.foo, original)

            new = m.foo = Mock()
            del m.foo
            self.assertEqual(m.foo, new)

    def test_attribute_deletion(self):
        for mock in Mock(), MagicMock():
            self.assertTrue(hasattr(mock, 'm'))

            del mock.m
            self.assertFalse(hasattr(mock, 'm'))

            del mock.f
            self.assertFalse(hasattr(mock, 'f'))
            self.assertRaises(AttributeError, getattr, mock, 'f')

They are testing the same thing but with different expectations. The first one is invalid and should be removed. The patch altered a bit of the second test to incorporate more mock classes.
msg197542 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2013-09-12 21:26
Good catch - thanks!
msg197543 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2013-09-12 21:28
Although I'm not certain the first test is "invalid". It's testing a different case than the second test. So if the first test passes it should be renamed rather than removed. If it *fails* then I'd like to look at the behaviour and specify (test) that.
msg197545 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-09-13 01:41
The first test fails. I already tested it.

I think they test the same thing, what happens to the attribute of mock instance if you delete it.
msg197564 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2013-09-13 12:20
Well, they actually test slightly different scenarios - they're *not* just duplicates of each other.
msg197615 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-09-13 17:26
Okay, I found the difference.

The second test case is to test the case on which we delete the attribute of the mock instance before being referenced by any variable. The we make sure the attribute is gone for good.

The first test case is to test the case on which we delete the attribute of the mock instance after being referenced by any variable (or maybe just needed to be used by __getattr__), then we make sure the attribute is resetted to the original value, not deleted.

I oversight this because I never thought that people could rely on this behaviour.
msg197691 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2013-09-14 07:14
The duplicate_code_names.py script in issue 16079 did find that there
are duplicate test_attribute_deletion tests in testmock.py (see
http://bugs.python.org/file27376/std_lib_duplicates.txt).
msg197695 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2013-09-14 08:27
On balance I think that this test failing is *correct* (after deleting an attribute the old mock should *not* come back). I'd rather adjust the test to assert this than just remove it though.
msg197696 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-09-14 09:05
Michael, attached the patch to accommodate your request. Yeah, I think this is the best way because I have seen no possible solutions to make both tests work without altering them.

Xavier, that's cool. I didn't know such tool exists. Usually I found the duplicate function/method by using flake plugin in Vim.
msg223427 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-07-18 19:30
Can we have a formal review on the latest patch please.
msg227425 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2014-09-24 08:57
This has been fixed in https://hg.python.org/cpython/rev/c8c11082bd0c.
History
Date User Action Args
2022-04-11 14:57:50adminsetgithub: 63193
2014-09-24 08:57:41berker.peksagsetstatus: open -> closed

nosy: + berker.peksag
messages: + msg227425

resolution: out of date
stage: resolved
2014-07-18 19:30:13BreamoreBoysetversions: + Python 3.5, - Python 3.3
nosy: + BreamoreBoy

messages: + msg223427

type: enhancement -> behavior
2013-09-14 09:06:00vajraskysetfiles: + fix_overshadowed_test_in_testmock.patch

messages: + msg197696
2013-09-14 08:27:55michael.foordsetmessages: + msg197695
2013-09-14 07:14:33xdegayesetnosy: + xdegaye
messages: + msg197691
2013-09-13 17:26:01vajraskysetmessages: + msg197615
2013-09-13 12:20:25michael.foordsetmessages: + msg197564
2013-09-13 01:41:24vajraskysetmessages: + msg197545
2013-09-12 21:28:36michael.foordsetassignee: michael.foord
messages: + msg197543
versions: + Python 3.3
2013-09-12 21:26:33michael.foordsetmessages: + msg197542
2013-09-12 17:36:33pitrousetnosy: + ezio.melotti, michael.foord
2013-09-10 06:55:03vajraskycreate