Fixed #27391 -- Implemented SimpleTestCase.debug().

[asfaltboy]

https://code.djangoproject.com/ticket/27391

Users calling debug() expect it to bubble up an exception if occurring in test, but behave the
same as run() when no exception occurred.

[timgraham]

Did you consider trying to factor the shared logic with __call__() to avoid the repetition?

[asfaltboy]

I was really trying to avoid it, I'd hate to see current simplicity get bogged down... but, how about this:

def _setup_and_call(self, result=None, debug=False):
    """
    Wrapper around default __call__ method to perform common Django test
    set up. This means that user-defined Test Cases aren't required to
    include a call to super().setUp().

    If passed debug=True, does does not catch errors and calls super().debug
    instead of __call__.
    """

    testMethod = getattr(self, self._testMethodName)
    skipped = (
        getattr(self.__class__, "__unittest_skip__", False) or
        getattr(testMethod, "__unittest_skip__", False))

    if not skipped:
        try:
            self._pre_setup()
        except Exception:
            result.addError(self, sys.exc_info())
            if debug:
                raise
            return
    if debug:
        super(SimpleTestCase, self).debug()
    else:
        super(SimpleTestCase, self).__call__(result)

    if not skipped:
        try:
            self._post_teardown()
        except Exception:
            result.addError(self, sys.exc_info())
            if debug:
                raise
            return

We can refactor this into smaller components obviously, if you prefer.

Update: simplified by removing contextmanager, it's actually shorter and avoids a bug where test is executed even if setup failed.

Update2: also found and amended an issue with running the skip test since in debug it raised SkipTest. Pushed again to run test suite.

[asfaltboy]

@timgraham please take a look at the updated code. Is this more or less what you envisioned? Or would you like anything changed here?

Also, should we mention this in docs/changelog ?

[timgraham]

I pushed a few updates -- some clean up remains if you could polish. Try to split the tests up rather than having one bit test. The tests that don't involve debug() are probably already passing before this change and so could be added in a separate commit that precedes the commit that adds debug().

[timgraham]

@asfaltboy, do you have any time and interest in completing this soon?

[asfaltboy]

Hi @timgraham sorry for taking long to respond (and for hogging the CI machine with amends).

How does the test case look now?

[timgraham]

It looks like there's a test isolation problem (see the build failures).

[asfaltboy]

Thanks @timgraham I'll try to address it later today or this week. Time constraints, sorry ...

[asfaltboy]

@timgraham it took "a bit" longer than promised, but I finally got to it.

The isolation issue was due to raised exception in Debug requiring to properly cleaning up after we're done with suite.

It's not a coincidence, because this is exactly what the fix intends to do, to let the test running framework (e.g pytest) to be able to catch errors and do their own cleanup as desired.

[timmartin]

Other than the exception issue above, this looks good to me. Perhaps this deserves an entry in the release notes?

[asfaltboy]

@timmartin I added a commit & comment for the mentioned case.

About updating release notes, I think we can rebase this PR over latest master and add this under 2.0.txt: Miscellaneous (currently line 634):

+* Implement the ``SimpleTestCase.debug()`` method to allow a test to run without
+  collecting the result. This allows exceptions raised by the test to be propagated to
+  the caller, and is used to support running tests under a debugger.
+  (:ticket:`27391`).

[timgraham]

Test coverage seems lacking as I commented out the two

if debug:
    raise

blocks and don't see any test failures.

[asfaltboy]

Hi @timmartin @timgraham , I've had some time during the europython event to address the requested changes. I've had to move the addError call to under the raise call in both pre-set and post-teardown clauses (which makes sense) and added tests for those.

The sqlite3-windows tests were failing on CI so I rebased (maybe un-necessarily) and now the sqlite3-bionic test is failing. Is this unrelated?

[timgraham]

Now I can comment both result.addError(self, sys.exc_info()) without seeing any test failures.

Yes, the build failure is unrelated. Don't worry about that.

[LuRsT]

Looks good, can we get this merged @Ian-Foote

[LilyAcorn]

This probably needs rebasing first, so it can be re-tested.

[asfaltboy]

@Ian-Foote @LuRsT thanks for your interest in merging this PR, it motivated me to get back to it.

I went ahead and rebased over latest master, and addressed the missing tests mentioned by @timgraham. These tests validate that the existing TestCase behavior, of capturing and logging errors in pre_setup & post_teardown, is retained after this change. (waiting for CI to confirm this runs correctly as tests fail on my machine fail erratically)

Is there anything we can address? Shall I squash the older WIP commits or leave them there? If nothing else, shall we mention this feature in release notes or docs?

[asfaltboy]

The failing test seems to be unrelated as it failed to check out the "PR merge commit", maybe this is an issue with the git plugin like the one described here ?

[LilyAcorn]

@carltongibson @felixxm

[felixxm]

@asfaltboy Failing tests are related with force-push you don't need to worry about them.

It is a new feature so documentation (topics/testing/tools.txt) and release notes are required.

[asfaltboy]

Thanks @felixxm, I doc'd the method in topics/testing/tools.txt and in releases/3.0.txt.

I feel, however, that this may be "too low-level" for the tools docs.
The target audience is Django test tool developers (pytest-django, nose, etc...), rather than Django users. I see this as the same reason that we don't document SimpleTestCase.run(), or am I wrong and we should have documented run() in the first place 🙂?

[felixxm]

@asfaltboy I moved extra tests to a separate PR #11936.

[felixxm]

@asfaltboy Thanks for updates 👍 I rebased from master, squashed commits. removed the SimpleTestCase.debug() documentation, added a versionchanged admonition, updated release notes and pushed minor edits. It's ready 🚀