testing-cabal / testtools

Testtools - tasteful testing for python
https://testtools.readthedocs.io/en/latest/
Other
95 stars 88 forks source link

Become compatible with newer Fixtures _setUp() API. #167

Closed bigjools closed 8 years ago

bigjools commented 8 years ago

Previously, when gathering details caused by a setUp failure, a traceback occurred if the fixture used the newer _setUp(). This also had the side effect of not clearing up fixtures properly.

Fixes: https://bugs.launchpad.net/testtools/+bug/1469759

Review on Reviewable

bigjools commented 8 years ago

I initially removed the whole try/except block but it kills support for the older Fixtures setUp() API. I can change it back if you want to do that. However, inspecting private properties on the fixture isn't so great either.

bigjools commented 8 years ago

Ping, is anyone looking at this please?

thomir commented 8 years ago

Hi,

I suspect the reason this hasn't had the attention it deserves is due to the failing travis run. @rbtcollins Fixed that, so I guess we need to re-trigger a travis run. I'm not sure how to do that, perhaps Robert has an idea?

rbtcollins commented 8 years ago

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


testtools/testcase.py, line 675 [r1] (raw file): This should be 'if safe_hasattr(fixture, '_details') and fixture._details is not None', since we can't trust any state in the failed fixture.

You also need to thread something like https://github.com/testing-cabal/fixtures/blob/master/fixtures/fixture.py#L258 in here, because we're not capturing the fixtures in the new exception either.


Comments from the review on Reviewable.io

bigjools commented 8 years ago

Reviewable is confusing me. Commenting here instead.

"You also need to thread something like https://github.com/testing-cabal/fixtures/blob/master/fixtures/fixture.py#L258 in here, because we're not capturing the fixtures in the new exception either."

Sorry, I don't understand what you mean.

bigjools commented 8 years ago

Ok I think I addressed all the concerns, let me know how it looks.

rbtcollins commented 8 years ago

Very close to mergable. One little tweak needed, plus please add an entry to NEWS and yourself to AUTHORS if you are not already there.


Reviewed 2 of 2 files at r4. Review status: all files reviewed at latest revision, 2 unresolved discussions.


testtools/testcase.py, line 685 [r4] (raw file): You need to either unconditionally import fixtures, or guard this with 'fixtures is not None and'


Comments from the review on Reviewable.io

bigjools commented 8 years ago

I thought about guarding the conditional fixtures import before, but it's impossible to even be in that code path without it. I've added the guard anyway as there's nothing to lose.

Thanks for reviewing, let me know how it looks now!

rbtcollins commented 8 years ago

Reviewed 3 of 3 files at r6. Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from the review on Reviewable.io

rbtcollins commented 8 years ago

Merged