terminal: fix progress report with duplicate nodeids

[blueyed]

Fixes #6597.

TODO:

• after terminal: refactor, no yellow ("boring") for non-last item #6409
• test with tests having an error during "setup" already.
• adjust test to reliably use duplicate nodeids? (Progression is wrong when using empty string in parametrized tests #6597 (comment))

[blueyed]

be6a7cd changes a lot, but is actually a bit of a revert, see the commit message.
I think it's better to just have a count, and not to use a set() as a "workaround", and consider this to be ready - assuming tests pass.

[bluetech]

According to this comment by @nicoddemus, duplicate nodeids are no longer possible. The change (set -> counter) might still make sense, but maybe the test doesn't?

[blueyed]

@bluetech nothing prevents you from creating duplicate nodeids. The test here uses a private property, but you could also yield Items with duplicate nodeids.

The test is for a corner case, yes, but makes sense.

It's also unfortunate that the patch is so big, but only because it was not done properly/better before and it has to undo previous changes (IIRC).

[bluetech]

Some lazy questions that would help me understand the diff -- feel free to dismiss.

Considering each change I can detect separately:

1. Changing when progress is indicated from if verbosity > 0 to if rep.when == "call" or (rep.when == "setup" and rep.failed). I understand why you removed it from the verbosity check - incrementing a counter doesn't cost anything so no need for it. But what are these new conditions?

2. Changing from set -> counter. This seems to make sense regardless, why use a heavy data structure when it's not needed? But the question is why was a set used in the first place? What it because it was assumed that no duplicate IDs are possible, or because it explicitly wanted to handle duplicate IDs by deduplicating them?

3. Changing from pytest_runtest_logfinish to pytest_runtest_teardown. IIUC these two should be mostly equivalent for this use case -- is it not so? If not, why? I possible I think this change should be avoided, mostly because the "log" hooks seem to exist particularly for this use case (reporting), and the code already uses logstart and logreport and it seems nice to use the matching logfinish hook.

4. Change from checking _is_last_item in pytest_runtest_logfinish hook to unconditionally running in pytest_runtestloop hookwrapper. This seems nice, but I'm not sure it's worth it as long as _is_last_item still exists. If you can get rid of _is_last_item entirely in favor the hook, that would be a nice cleanup.

[nicoddemus]

Shouldn't this come after if not letter and not word:? Because "probably passed setup/teardown" should not be reported again.

[nicoddemus]

I'm curious, why using pytest_runtest_teardown instead of pytest_runtest_logfinish? On pytest-xdist I think pytest_runtest_teardown is called only on worker nodes (because they actually run the tests), not on master (which only does reporting). The pytest_runtest_log* run on both master and workers, by design.

[nicoddemus]

Hmmm strange, I commented out this function from your branch, and I still get the exact same output both with xdist/without, and -v and normal modes.

---

With the patch as is:

λ pytest testing\test_nodes.py
======================== test session starts ========================
collected 11 items

testing\test_nodes.py ...........                              [100%]

======================== 11 passed in 0.10s =========================

Commenting out pytest_runtest_teardown:

 λ pytest testing\test_nodes.py
======================== test session starts ========================
collected 11 items

testing\test_nodes.py ...........                              [100%]

======================== 11 passed in 0.10s =========================

---

With the patch as is:

λ pytest testing\test_nodes.py -n2
======================== test session starts ========================
gw0 [11] / gw1 [11]
...........                                                    [100%]
======================== 11 passed in 0.93s =========================

Commenting out pytest_runtest_teardown:

λ pytest testing\test_nodes.py -n2
======================== test session starts ========================
gw0 [11] / gw1 [11]
...........                                                    [100%]
======================== 11 passed in 0.92s =========================

---

So I guess pytest_runtest_teardown is not being used?

I got curious, and when in master and pytest_runtest_logfinish commented out, the [%] progress indicator is not printed.

[blueyed]

The pytest_runtest_log* run on both master and workers, by design.

That might result in too much being reported then (if both master and nodes report the same item(s)).
Changed it to use pytest_runtest_logfinish again, let's see if it passes.

[nicoddemus]

I think we should avoid attributes being set but not declared on __init__ when that is possible; this has been a great source of confusion/bugs in the past.

[nicoddemus]

But the question is why was a set used in the first place? What it because it was assumed that no duplicate IDs are possible, or because it explicitly wanted to handle duplicate IDs by deduplicating them?

IIRC the former, because it is easier/more solid to just use the node ids if they are unique. But indeed, there's nothing preventing plugins to create items with duplicated ids, so using a set here is not really the right call.

[blueyed]

@nicoddemus @bluetech
Please squash-merge or close it.

[blueyed]

(btw: nodeids were generated automatically not too long ago, which is actually something I've came up with somewhere already: 5e59357)