In two "test_copying" methods in Lib/test/test_collections.py, variable i is never used. My guess is the original test writer forgot to utilize the variable i.

For example, in test_copying method in TestOrderedDict class:

    def test_copying(self):
        # Check that ordered dicts are copyable, deepcopyable, picklable,
        # and have a repr/eval round-trip
        pairs = [('c', 1), ('b', 2), ('a', 3), ('d', 4), ('e', 5), ('f', 6)]
        od = OrderedDict(pairs)
        update_test = OrderedDict()
        update_test.update(od)
        for i, dup in enumerate([
                    od.copy(),
                    copy.copy(od),
                    copy.deepcopy(od),
                    pickle.loads(pickle.dumps(od, 0)),
                    pickle.loads(pickle.dumps(od, 1)),
                    pickle.loads(pickle.dumps(od, 2)),
                    pickle.loads(pickle.dumps(od, 3)),
                    pickle.loads(pickle.dumps(od, -1)),
                    eval(repr(od)),
                    update_test,
                    OrderedDict(od),
                    ]):
            self.assertTrue(dup is not od)
            self.assertEqual(dup, od)
            self.assertEqual(list(dup.items()), list(od.items()))
            self.assertEqual(len(dup), len(od))
            self.assertEqual(type(dup), type(od))

The variable i in "for i, dup in enumerate" is never used.

The test_copying method in TestCounter class has the same problem.

In my opinion, we need to put variable i inside the message in the assert functions to detect which place inside the iteration the test fails.

Perhaps it will be even better to extract loop body as a local function and then call it with different arguments.

    def check(dup):
        self.assertTrue(dup is not od)
        self.assertEqual(dup, od)
        ...
    check(od.copy())
    check(copy.copy(od))
    ...

In this case we will see a tested case right in the traceback.

According to R. David Murray in Python core-mentorship mailing list addressing me:

"It could be that the bug is that the i is not used...it may have been
intended to be used in an extended error message in the asserts, so that
it would be clear which input failed.  In any case, I think the best
fix here would probably be to use the new subtests support in unittest."

So I used subTest feature in the second patch I upload according to his advice.

What do you think? subTest can recognize where the test fails immediately as well. You just have to count the line in the loop.

Anyway to make it complete, I upload the patch according to Storchaka's advice too.

May the best patch wins!

To me, there are two independent improvements: using i and hence msg to identify the erroneous dup, and using .subTest to test all dups. I would combine ideas from both the first two patches to do both.

1. a fancy formatted message would be more useful with less fluff and the two displays each on their own line.

msg = "iteration %s\ncopy: %s\norig: %s" % (i, dup, words)
# or od for OrderedDict test

2. within both loops, add msg to all asserts as in the first patch. If there is a failure, one would want to know both the source of the dup (indicated by i) and the content, and how it differed from the model.

3. wrap the bodies of both loops with 'with self.subTest():' to always test all dups. Unrolling the loops prevents doing this.

Test the revised tests by temporarily adding both the original (failing the 'is' test) and a partial and therefore faulty copy of the original to both list of dups. Verify that both errors are reported for each and that the message formatting looks ok.

subTest() serves two purposes: identify the test case and continue testing
oother test cases after a fail. For first purpose subTest() is not well
suitable in these tests, becouse we have no a string which determine a
test. Second purpose is not relevant to original issue.

1. a fancy formatted message would be more useful with less fluff and the
> two displays each on their own line.
>
> msg = "iteration %s\ncopy: %s\norig: %s" % (i, dup, words)
> # or od for OrderedDict test
>

When the test fails, dup is not expected value and printing it can raise an
exception and prevent formatting useful error message. Failure report
should be succesful in any case.

3. wrap the bodies of both loops with 'with self.subTest():' to always test
> all dups. Unrolling the loops prevents doing this.
>

Of course you can wrap a body of the check() function.

> becouse we have no a string which determine a test.

The original loop could be have been written as (or be changed to)

for label, dup in [
        ('odcopy', od.copy()),
        ('copycopy', copy.copy(words)),
        <and so on>
        ]
and the label used to identify the test, whether passed to assertX or subTest. The test author (Raymond H.) thought the sequence number enough in the off chance there ever were a failure.

>When the test fails, dup is not expected value and printing it can raise an exception and prevent formatting useful error message.

I do not understand this, unittest assumes that tested objects are printable. Indeed, assertEqual(dup, words/od) will do that (making the message redundant for this particular test.

So I think the best fix would be to redo the loop header as above, pass label to subTest so it appears for any failure (which intertwines subTest with this issue), and only pass a message to assert where actually needed (like the len test).

I redo the test as Terry J. Reedy suggested. I use label in loop, utilize subTest receiving label parameter, and pass messages to only necessary asserts.

So roughly if the test fails, we would get something like this:

======================================================================
FAIL: test_copying (__main__.TestOrderedDict) (label='OrderedDict(od)')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "Lib/test/test_collections.py", line 1237, in test_copying
    self.assertTrue(dup is not dup, msg)
AssertionError: False is not true : 
copy: OrderedDict([('c', 1), ('b', 2), ('a', 3), ('d', 4), ('e', 5), ('f', 6)])
od: OrderedDict([('c', 1), ('b', 2), ('a', 3), ('d', 4), ('e', 5), ('f', 6)])

======================================================================
FAIL: test_copying (__main__.TestCounter) (label='Counter(words)')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "Lib/test/test_collections.py", line 942, in test_copying
    self.assertEqual(type(dup), type(''), msg)
AssertionError: <class 'collections.Counter'> != <class 'str'> : 
copy: Counter({'which': 2, 'witches': 1, 'witch': 1, 'watch': 1, 'wrist': 1, 'had': 1})
words: Counter({'which': 2, 'witches': 1, 'had': 1, 'watch': 1, 'witch': 1, 'wrist': 1})

Fixed the test based on Ezio Melotti's advice.

However, Ezio did not comment specifically about whether we should cut or keep this line.

self.assertEqual(list(dup.items()), list(od.items()))

After studying the OrderedDict source code, I came to conclusion that if "self.assertEqual(dup, od)" is true, so is "self.assertEqual(list(dup.items()), list(od.items()))".

But if "self.assertEqual(dup, od)" is false, so is "self.assertEqual(list(dup.items()), list(od.items()))".

This is how OrderedDict tests the equality:

    def __eq__(self, other):
        if isinstance(other, OrderedDict):
            return dict.__eq__(self, other) and all(map(_eq, self, other))
        return dict.__eq__(self, other)

So I think it should be safe to remove:

self.assertEqual(list(dup.items()), list(od.items()))

New changeset a5010de76eda by Ezio Melotti in branch 'default':
#18106: refactor tests to use subtests and proper assert methods.  Patch by Vajrasky Kok.
http://hg.python.org/cpython/rev/a5010de76eda

Fixed, thanks for the patch!