[FIX] Added cleaning steps for cashed fixture result and failed setup states. #66

OlegKuzovkov · 2018-09-08T01:34:38Z

fixes #64 issue for:

setup_class method
session\module\class scope fixtures

…up states.

…cover the fixed cases.

icemac

LGTM.

sallner

@OlegKuzovkov Thank you for your contribution. I added some comments and hope you can address them. Additionally this should find an entry in the changelog and a short mention in the readme, as it seems now possible to have the rerun on class scoped fixtures.

sallner · 2018-09-14T12:29:14Z

pytest_rerunfailures.py

+                delattr(fixture_def, cached_result)
+
+
+def _remove_failed_setup_state_from_session(item):


I am a little bit reluctant to introduce cleanup login in the plugin. This could lead to too much coupling. As the SetupState is not too complicated, it looks like https://github.com/pytest-dev/pytest/blob/master/src/_pytest/runner.py#L331 is actually doing the job already. Would that be a possibility?

If I understand correctly, your advice is to use SetupState.teardown_all() method, instead of a custom cleaner that I've implemented.

I've tried to use it, but it seems not handling the resource properly. All it does is calling all the teadowns for each col elements in the stack, but it is missing the most important part: invalidation/removing the failed execution result stored in _prepare_exc field. That's why every new test that is relying on setup_class will rerun it anyway, but it shouldn't because setup was already successfully re-executed after the first attempt.
You can see the problem by changing the body of _remove_failed_setup_state_from_session function to:

setup_state = getattr(item.session, '_setupstate') setup_state.teardown_all()

And running the test:

class TestFoo(object): execution_number = 1 srr = None @classmethod def setup_class(cls): cls.srr = Random.rand_char_string(10) Log.info('IN SETUP %s' % cls.srr) if cls.execution_number: cls.execution_number -= 1 assert False assert True @mark.parametrize('param', [1, 2, 3]) def test_pass(self, param): Log.info('IN TEST %s %s' % (param, self.srr)) assert param

The output will be:

R2018-09-14 11:12:27,314 IN SETUP XWnfETJrmi 2018-09-14 11:12:27,568 IN SETUP ArdcndHZkT .2018-09-14 11:12:27,569 IN TEST 1 ArdcndHZkT R2018-09-14 11:12:27,617 IN SETUP GbnwDwvSWi .2018-09-14 11:12:27,618 IN TEST 2 GbnwDwvSWi R2018-09-14 11:12:27,642 IN SETUP tdWVHVPPyi .2018-09-14 11:12:27,643 IN TEST 3 tdWVHVPPyi

I do agree with you though, that instead of cleaning stack by myself, I should've used teardown_all().
With this change the output of the previous test is correct:

R2018-09-14 11:13:36,147 IN SETUP biCSDlQwSP 2018-09-14 11:13:36,303 IN SETUP ZKlZjHlYDB .2018-09-14 11:13:36,304 IN TEST 1 ZKlZjHlYDB .2018-09-14 11:13:36,308 IN TEST 2 ZKlZjHlYDB .2018-09-14 11:13:36,320 IN TEST 3 ZKlZjHlYDB

Further investigation showed that we cannot use .teardown_all() because it is triggering all the finalizers, even for the module/session scoped fixtures. Thus, leaving original implementation.

Yeah, that is true. So I am happy with keeping the cleanup here. At this point a comment could be helpful.

sallner · 2018-09-14T12:46:10Z

pytest_rerunfailures.py

@@ -160,6 +185,10 @@ def pytest_runtest_protocol(item, nextitem):
                    # will rerun test, log intermediate result
                    item.ihook.pytest_runtest_logreport(report=report)

+                # cleanin item's cashed results from any level of setups
+                _remove_cached_results_from_fixtures(item)
+                _remove_failed_setup_state_from_session(item)


What I am not sure about is the effect for the result of a module or session scoped fixture. Could you please add a test for them as well, that they are not affected by this change? I guess this should not re-initiate those fixtures as they could be expensive database fixtures.

This indeed will invalidate all the fixture cached_results. At this point I have a question for you:
If the test failed, do we want to rerun all fixtures or just the one that failed?
I've implemented this way on purpose because I think that if something goes wrong, It is better to reinitialize all the dependencies. But now I have some doubts about it, because of the reason you've mentioned.
Based on your answer I will change the implementation accordingly.

I would not like to re-initialize all fixtures, but only the one who failed. So that should be the case with the current state of the PR.

That is correct. The only failed fixture will be erased from any previous results.

sallner · 2018-09-14T12:51:37Z

test_pytest_rerunfailures.py

+    assert_outcomes(result, passed=3, rerun=1)
+
+
+def test_rerun_on_class_scope_fixture_setup_with_first_execution_error_for_parametrized_test(testdir):


The function names of these tests are quite long. Could you please shorten them and write the more verbose description in a docstring? This reminds me to add a flake8 test to the tox run.

I will be really appreciated if you could suggest me the names for these cases. I really cannot find any clearer and shorter test names for this specific contexts :)

OlegKuzovkov · 2018-09-14T19:21:53Z

Thank you for your feedback. I've tried to change/fix everything you've mentioned. If you find anything else - let me know.
Thank you.

sallner

This should be fine now. If you find time, you could maybe consider the comments on test. As I am not connected very well this week, I plan to merge and release the changes next week.

sallner · 2018-09-17T16:25:58Z

pytest_rerunfailures.py

-                delattr(fixture_def, cached_result)
+                result, cache_key, err = getattr(fixture_def, cached_result)
+                if err:  # Deleting cached results for only failed fixtures
+                    delattr(fixture_def, cached_result)


I like the test for an error. Hopefully we will not have a chain of dependent fixtures and strange results, but then the dependent fixture should also produce an error.

I've added the test for the error (test_rerun_on_module_fixture_with_reruns). Only setup_fixture in that scenario will fail and will be cleaned up. Dependent fixture cannot produce any error if one of the fixtures before fails.
Unless I am missing something.

sallner · 2018-09-17T16:27:21Z

pytest_rerunfailures.py

+                delattr(fixture_def, cached_result)
+
+
+def _remove_failed_setup_state_from_session(item):


Yeah, that is true. So I am happy with keeping the cleanup here. At this point a comment could be helpful.

sallner · 2018-09-17T16:28:57Z

pytest_rerunfailures.py

@@ -160,6 +185,10 @@ def pytest_runtest_protocol(item, nextitem):
                    # will rerun test, log intermediate result
                    item.ihook.pytest_runtest_logreport(report=report)

+                # cleanin item's cashed results from any level of setups
+                _remove_cached_results_from_fixtures(item)
+                _remove_failed_setup_state_from_session(item)


I would not like to re-initialize all fixtures, but only the one who failed. So that should be the case with the current state of the PR.

sallner · 2018-09-17T16:29:25Z

test_pytest_rerunfailures.py

+def test_rerun_on_class_scope_fixture_with_error_with_reruns(testdir):
+    """
+    Case: Class scope fixture throwing error on the first execution for parametrized test
+    """


Thank you for rephrasing the function names.

sallner · 2018-09-17T16:31:39Z

test_pytest_rerunfailures.py

+        import pytest
+
+        class Execution:
+            count = 0


I like those two tests and I see that you have this pattern of execution number, but I seems more pythonic to me to use a global boolean in this case instead of a singleton class to store it. But that is on you.

Thank you for the suggestion. Will redo this.

…class with static field according to PR comment.

# Conflicts: # CHANGES.rst # test_pytest_rerunfailures.py

[FIX] Added cleaning steps for cashed fixtures results and failed set…

7bce517

…up states.

OlegKuzovkov changed the title ~~[FIX] Added cleaning steps for cashed fixtures results and failed setup states.~~ [FIX] Added cleaning steps for cashed fixture result and failed setup states. Sep 8, 2018

[FIX] Fixed issue with setup_class. [TEST] Added 2 new unit tests to …

fc1d829

…cover the fixed cases.

icemac approved these changes Sep 11, 2018

View reviewed changes

icemac assigned sallner Sep 11, 2018

sallner requested changes Sep 14, 2018

View reviewed changes

OlegKuzovkov added 2 commits September 14, 2018 14:58

[FEAT] Requested RP changes.

694f471

[DOC] Added entries into changes and readme files.

ea60e4a

[FIX] Fixed clearing stack approach.

44d9d63

sallner approved these changes Sep 17, 2018

View reviewed changes

OlegKuzovkov added 2 commits September 17, 2018 13:20

[REFACT] Refactored test cases to use a global variable instead of a …

045f46c

…class with static field according to PR comment.

Merge branch 'master' into pytest-dev#64-issue-fix

39a95ee

# Conflicts: # CHANGES.rst # test_pytest_rerunfailures.py

sallner merged commit 00e3333 into pytest-dev:master Oct 4, 2018

knedlsepp mentioned this pull request Mar 23, 2022

Clarification of class/module/package level fixture support #183

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Added cleaning steps for cashed fixture result and failed setup states. #66

[FIX] Added cleaning steps for cashed fixture result and failed setup states. #66

OlegKuzovkov commented Sep 8, 2018 •

edited

Loading

icemac left a comment

sallner left a comment

sallner Sep 14, 2018

OlegKuzovkov Sep 14, 2018 •

edited

Loading

OlegKuzovkov Sep 14, 2018

sallner Sep 17, 2018

sallner Sep 14, 2018

OlegKuzovkov Sep 14, 2018

sallner Sep 17, 2018

OlegKuzovkov Sep 17, 2018

sallner Sep 14, 2018

OlegKuzovkov Sep 14, 2018

OlegKuzovkov commented Sep 14, 2018

sallner left a comment

sallner Sep 17, 2018

OlegKuzovkov Sep 17, 2018

sallner Sep 17, 2018

sallner Sep 17, 2018

sallner Sep 17, 2018

sallner Sep 17, 2018

OlegKuzovkov Sep 17, 2018

		delattr(fixture_def, cached_result)


		def _remove_failed_setup_state_from_session(item):

		assert_outcomes(result, passed=3, rerun=1)


		def test_rerun_on_class_scope_fixture_setup_with_first_execution_error_for_parametrized_test(testdir):

[FIX] Added cleaning steps for cashed fixture result and failed setup states. #66

[FIX] Added cleaning steps for cashed fixture result and failed setup states. #66

Conversation

OlegKuzovkov commented Sep 8, 2018 • edited Loading

icemac left a comment

Choose a reason for hiding this comment

sallner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegKuzovkov Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegKuzovkov commented Sep 14, 2018

sallner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegKuzovkov commented Sep 8, 2018 •

edited

Loading

OlegKuzovkov Sep 14, 2018 •

edited

Loading