terminal: fix progress report with duplicate nodeids #6599

blueyed · 2020-01-28T14:37:12Z

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))

src/_pytest/terminal.py

What's needed to fix pytest-dev#3088 seems to be checking for `rep.when == "setup"`. This then needs adjustments though (to code done in pytest-dev#3110) to make this work with xdist. It appears to make sense to split it into hooks that are run per test (for eventually writing if past edge), and one that gets run at the end really (not requiring to check for "last item" then).

TODO: - `test_xdist_verbose` fails: first two tests are reporting at 10% already. Conflicts: changelog/6597.bugfix.rst src/_pytest/terminal.py testing/test_terminal.py

blueyed · 2020-02-19T20:46:38Z

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.

src/_pytest/terminal.py

bluetech · 2020-02-28T09:31:35Z

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 · 2020-02-28T21:06:43Z

@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 · 2020-02-29T11:19:55Z

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

Considering each change I can detect separately:

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?
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?
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.
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 · 2020-03-04T11:31:00Z

src/_pytest/terminal.py

@@ -440,6 +440,8 @@ def pytest_runtest_logreport(self, report: TestReport) -> None:
        else:
            markup = None
        self._add_stats(category, [rep])
+        if rep.when == "call" or (rep.when == "setup" and rep.failed):


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

nicoddemus · 2020-03-04T11:33:35Z

src/_pytest/terminal.py

-                progress_length = len(" [{}/{}]".format(str(num_tests), str(num_tests)))
-            else:
-                progress_length = len(" [100%]")
+    def pytest_runtest_teardown(self) -> None:


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.

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.

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 · 2020-03-04T11:35:19Z

src/_pytest/terminal.py

+        """Write final progress indicator."""
+        yield
+        if (
+            getattr(self, "_tests_ran", False)


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 · 2020-03-04T11:53:53Z

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.

Upstream: pytest-dev#6599

blueyed · 2020-03-27T01:46:52Z

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

blueyed · 2020-03-31T08:09:09Z

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

blueyed mentioned this pull request Jan 28, 2020

Progression is wrong when using empty string in parametrized tests #6597

Open

blueyed force-pushed the fix-progress-same-nodeids branch from 859b0bb to f70ae66 Compare January 28, 2020 14:48

blueyed commented Jan 28, 2020

View reviewed changes

src/_pytest/terminal.py Outdated Show resolved Hide resolved

blueyed force-pushed the fix-progress-same-nodeids branch from b059834 to cc26d81 Compare January 28, 2020 15:26

blueyed mentioned this pull request Jan 28, 2020

tests: harden test_teardown_many_verbose #6600

Merged

blueyed changed the title ~~terminal: fix progress report with duplicate nodeids~~ [WIP] terminal: fix progress report with duplicate nodeids Jan 30, 2020

blueyed changed the title ~~[WIP] terminal: fix progress report with duplicate nodeids~~ [blocked] terminal: fix progress report with duplicate nodeids Jan 30, 2020

blueyed added 2 commits February 9, 2020 10:01

terminal: fix progress report with duplicate nodeids

f606077

Fixes pytest-dev#6597.

test_same_nodeids

aac97ef

blueyed force-pushed the fix-progress-same-nodeids branch from cc26d81 to aac97ef Compare February 9, 2020 09:07

count setups

a5606b4

blueyed changed the title ~~[blocked] terminal: fix progress report with duplicate nodeids~~ terminal: fix progress report with duplicate nodeids Feb 9, 2020

fixup! count setups

e8e6567

blueyed mentioned this pull request Feb 9, 2020

tests: harden test_xdist_verbose #6700

Merged

blueyed changed the title ~~terminal: fix progress report with duplicate nodeids~~ [WIP] terminal: fix progress report with duplicate nodeids Feb 9, 2020

blueyed added 3 commits February 10, 2020 21:24

count in pytest_runtest_logfinish

bbd80ef

assert

c3fcc9a

blueyed changed the title ~~[WIP] terminal: fix progress report with duplicate nodeids~~ [WIP/blocked] terminal: fix progress report with duplicate nodeids Feb 10, 2020

blueyed added 2 commits February 10, 2020 23:27

make use of never used _tests_ran

cad0319

Merge branch 'master' into fix-progress-same-nodeids

f09505d

TODO: - `test_xdist_verbose` fails: first two tests are reporting at 10% already. Conflicts: changelog/6597.bugfix.rst src/_pytest/terminal.py testing/test_terminal.py

blueyed changed the title ~~[WIP/blocked] terminal: fix progress report with duplicate nodeids~~ [WIP/] terminal: fix progress report with duplicate nodeids Feb 18, 2020

blueyed changed the title ~~[WIP/] terminal: fix progress report with duplicate nodeids~~ [WIP] terminal: fix progress report with duplicate nodeids Feb 18, 2020

blueyed added 3 commits February 18, 2020 21:08

Merge branch 'master' into fix-progress-same-nodeids

9d19f88

or (rep.when == "setup" and rep.failed)

dbd3f46

less diff churn

f660904

blueyed changed the title ~~[WIP] terminal: fix progress report with duplicate nodeids~~ terminal: fix progress report with duplicate nodeids Feb 19, 2020

bluetech reviewed Feb 28, 2020

View reviewed changes

src/_pytest/terminal.py Show resolved Hide resolved

nicoddemus reviewed Mar 4, 2020

View reviewed changes

s/pytest_runtest_teardown/pytest_runtest_logfinish

b6c7c51

blueyed added a commit to blueyed/pytest that referenced this pull request Mar 14, 2020

terminal: fix progress report with duplicate nodeids

fa78dbf

Upstream: pytest-dev#6599

blueyed mentioned this pull request Mar 14, 2020

terminal: fix progress report with duplicate nodeids blueyed/pytest#293

Merged

blueyed added a commit to blueyed/pytest that referenced this pull request Mar 14, 2020

terminal: fix progress report with duplicate nodeids (#293)

63b9cfd

Upstream: pytest-dev#6599

nicoddemus closed this May 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

terminal: fix progress report with duplicate nodeids #6599

terminal: fix progress report with duplicate nodeids #6599

blueyed commented Jan 28, 2020 •

edited

Loading

blueyed commented Feb 19, 2020

bluetech commented Feb 28, 2020

blueyed commented Feb 28, 2020

bluetech commented Feb 29, 2020

nicoddemus Mar 4, 2020

nicoddemus Mar 4, 2020

nicoddemus Mar 4, 2020

blueyed Mar 5, 2020

nicoddemus Mar 4, 2020

nicoddemus commented Mar 4, 2020

blueyed commented Mar 27, 2020

blueyed commented Mar 31, 2020

terminal: fix progress report with duplicate nodeids #6599

terminal: fix progress report with duplicate nodeids #6599

Conversation

blueyed commented Jan 28, 2020 • edited Loading

blueyed commented Feb 19, 2020

bluetech commented Feb 28, 2020

blueyed commented Feb 28, 2020

bluetech commented Feb 29, 2020

nicoddemus Mar 4, 2020

Choose a reason for hiding this comment

nicoddemus Mar 4, 2020

Choose a reason for hiding this comment

nicoddemus Mar 4, 2020

Choose a reason for hiding this comment

blueyed Mar 5, 2020

Choose a reason for hiding this comment

nicoddemus Mar 4, 2020

Choose a reason for hiding this comment

nicoddemus commented Mar 4, 2020

blueyed commented Mar 27, 2020

blueyed commented Mar 31, 2020

blueyed commented Jan 28, 2020 •

edited

Loading