Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failure in TestExtractStuffAndWhy with Twisted 17.9 #144

Closed
vincentbernat opened this issue Oct 14, 2017 · 2 comments
Closed

Failure in TestExtractStuffAndWhy with Twisted 17.9 #144

vincentbernat opened this issue Oct 14, 2017 · 2 comments

Comments

@vincentbernat
Copy link
Contributor

@vincentbernat vincentbernat commented Oct 14, 2017

Hello,

Following bug #876988 in Debian, when I try to run tests with Twisted 17.9 installed, I get the following error:

================================================= FAILURES =================================================
_______________________________ TestExtractStuffAndWhy.test_handlesFailures ________________________________

self = <tests.test_twisted.TestExtractStuffAndWhy object at 0x7f2a8a4917d0>

    def test_handlesFailures(self):
        """
            Extracts failures and events.
            """
        f = Failure(ValueError())
>       assert (
            (f, "foo", {}) ==
            _extractStuffAndWhy({"_why": "foo",
                                 "_stuff": f})
        )
E       AssertionError: assert (<twisted.pyt... >, 'foo', {}) == (<twisted.pyth... >, 'foo', {})
E         At index 0 diff: <twisted.python.failure.Failure exceptions.ValueError: > != <twisted.python.failure.Failure exceptions.ValueError: >
E         Full diff:
E         (<twisted.python.failure.Failure exceptions.ValueError: >, 'foo', {})

tests/test_twisted.py:117: AssertionError
============================= 1 failed, 229 passed, 10 skipped in 0.50 seconds =============================

Putting some print arounds, the problem is that isinstance(_stuff, BaseException) is true, so we create a new encapsulate _stuff twice. I'll propose a fix shortly.

vincentbernat added a commit to vincentbernat/structlog that referenced this issue Oct 14, 2017
If a failure is already a Failure instance, don't encapsulate it into
a Failure. Keep it as is.

Fix hynek#144
@hynek
Copy link
Owner

@hynek hynek commented Oct 14, 2017

I don’t have the time to deal with it immediately, but it seems like it's related to:

I’ll need a moment to read/understand what’s actually going on.

@vincentbernat
Copy link
Contributor Author

@vincentbernat vincentbernat commented Nov 11, 2017

Any news? After looking at the first ticket, I think my fix is correct: Failure now inherits of BaseException and therefore, the condition should be updated. Maybe you are worried of other side-effects of this change?

vincentbernat added a commit to vincentbernat/structlog that referenced this issue Nov 14, 2017
If a failure is already a Failure instance, don't encapsulate it into
a Failure. Keep it as is.

Fix hynek#144
@hynek hynek closed this in #145 Nov 15, 2017
hynek added a commit that referenced this issue Nov 15, 2017
* twisted: don't encapsulate failures twice

If a failure is already a Failure instance, don't encapsulate it into
a Failure. Keep it as is.

Fix #144

* twisted: run tests with older versions of Twisted

In versions < 17, Failure is not a subclass of BaseException, while it
is in versions > 17.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants