Deprecations fixture

[brantlk]

A new Deprecations fixture is added which when active causes calls
to deprecated function to raise.

[rbtcollins]

This is triggering warnings in travis - fixtures/tests/_fixtures/test_deprecations.py:38: DeprecationWarning: message ignored

warnings.warn('message ignored', DeprecationWarning)

fixtures/tests/_fixtures/test_deprecations.py:45: DeprecationWarning: message ignored

warnings.warn('message ignored', DeprecationWarning)

fixtures/tests/_fixtures/test_deprecations.py:59: DeprecationWarning: message ignored

warnings.warn('message ignored', DeprecationWarning)

fixtures/tests/_fixtures/test_deprecations.py:54: DeprecationWarning: message ignored

warnings.warn('message ignored', DeprecationWarning)

[rbtcollins]

You'll need to import warnings here.

You can make this a little cleaner by writing the test using with (we no longer support 2.5)

>>> with fixture:
...    warnings.warn('stop using me', DeprecationWarning)
Exception...

[brantlk]

imported warnings and now local tox -e py27 works. Also switched to using context for the setUp/cleanUp.

[rbtcollins]

Hi, thanks for the patch, sorry for failing to review promptly. Please nag me on IRC in future!

Have you considered making the fixture wrap catch_warnings? That would fix the resetwarnings error, though users would have to be sure to nest Deprecations usage (which testtools.TestCase.useFixture() does naturally, so that should be fine), and would prevent stdout noise making tests hard to read at the same time.

---

Review status: 0 of 7 files reviewed at latest revision, 13 unresolved discussions.

---

fixtures/_fixtures/deprecations.py, line 13 [r2] (raw file):
Please see COPYING: fixtures is dual license BSD + Apache, and we provide a standard header to include.

---

fixtures/_fixtures/deprecations.py, line 14 [r2] (raw file):
The module needs an all listing Deprecations.

---

fixtures/_fixtures/deprecations.py, line 19 [r2] (raw file):
Please don't call the local module 'warnings' - humans will be confused too. warningfixture or something would be good.

---

fixtures/_fixtures/deprecations.py, line 25 [r2] (raw file):
s/function/functions here, because calls is plural; that or say 'a function', or 'a deprecation function' or something like that.

---

fixtures/_fixtures/deprecations.py, line 44 [r2] (raw file):
I think this would read better as: "The name of a Python module. DeprecationWarnings emitted from this module will cause an error to be raised.". Also, the :param: syntax doesn't accept a type as far as I know - ":param module:".

---

fixtures/_fixtures/deprecations.py, line 45 [r2] (raw file):
Trailing blank line here to remove.

---

fixtures/_fixtures/deprecations.py, line 56 [r2] (raw file):
Hmm, resetwarnings actually /resets/ it. I think we want to pop just the thing we added back out from the warning handler list.

e.g. I'd expect a test like:

with Deprecations(name):
warnings.filterwarnings('once', re.escape(name))
with catch_warnings(record=True) as output:
warnings.warn(Warning, "Fred")
self.expectThat(output, HasLength(1))

to fail.

---

fixtures/_fixtures/deprecations.py, line 63 [r2] (raw file):
Spurious line here.

---

fixtures/_fixtures/deprecations.py, line 65 [r2] (raw file):
This doesn't actually do what the docstring claims - if the default warnings are set to error on deprecations, it will actually still error. I am guessing but I think that what this is for is to temporarily turn off the fixture? Assuming we fixed the cleanup, another way would be to have a little context manager that exits-and-enters the fixture, and preserve a reference to it in your base class.

That said, having the function here is ok, but I think we'll want to tweak the name and docstring - since nothing will fail if you call expect_deprecations() and then don't trigger any deprecations.

---

fixtures/_fixtures/deprecations.py, line 84 [r2] (raw file):
So this has the common bug theme of calling resetwarnings. With that solved, I think you can make it leaner:

@contextlib.contextmanager
def expect_deprecations_here(self):
"""..."""
try:
self.cleanUp()
yield
finally:
self.setUp()

Again though, the name is misleading, as there is no actual expectation - this is limited to disabling the check, not to actually providing an expectation.

---

fixtures/tests/_fixtures/test_deprecations.py, line 14 [r2] (raw file):
Ditto on the header.

---

fixtures/tests/_fixtures/test_deprecations.py, line 59 [r2] (raw file):
This depends on variables outside our control - I don't think having such a case is appropriate. In particular options to Python itself control this.

This is missing tests that check the interaction with global state - e.g. having two separate Deprecations fixtures at the moment won't work properly because of resetwarnings.

---

Comments from the review on Reviewable.io

[rbtcollins]

ping

[brantlk]

Review status: 0 of 7 files reviewed at latest revision, 13 unresolved discussions.

---

fixtures/_fixtures/deprecations.py, line 13 [r2] (raw file):
Done.

---

fixtures/_fixtures/deprecations.py, line 14 [r2] (raw file):
Done.

---

fixtures/_fixtures/deprecations.py, line 19 [r2] (raw file):
warnings.py already exists as part of _fixtures, this is trying to import warnings from the stdlib... want me to change the name of the warnings module?

---

fixtures/_fixtures/deprecations.py, line 25 [r2] (raw file):
Done.

---

fixtures/_fixtures/deprecations.py, line 44 [r2] (raw file):
Updated. The :param: syntax does allow the type, see http://www.sphinx-doc.org/en/stable/domains.html#info-field-lists . Could also use :type module: str.

---

fixtures/_fixtures/deprecations.py, line 45 [r2] (raw file):
Done.

---

fixtures/_fixtures/deprecations.py, line 63 [r2] (raw file):
Done.

---

fixtures/tests/_fixtures/test_deprecations.py, line 14 [r2] (raw file):
Done.

---

Comments from Reviewable

[brantlk]

Review status: 0 of 6 files reviewed at latest revision, 13 unresolved discussions.

---

fixtures/_fixtures/deprecations.py, line 56 [r2] (raw file):
See what you think of the latest, it uses catch_warnings.

---

fixtures/_fixtures/deprecations.py, line 65 [r2] (raw file):
I updated it see what you think.

---

fixtures/_fixtures/deprecations.py, line 84 [r2] (raw file):
Done.

---

Comments from Reviewable

[brantlk]

Review status: 0 of 6 files reviewed at latest revision, 13 unresolved discussions.

---

fixtures/tests/_fixtures/test_deprecations.py, line 59 [r2] (raw file):
Test added.

---

Comments from Reviewable

[rbtcollins]

This is looking pretty close. One aesthetic thing - this is dealing with warnings, so I'd really like to see the code for it be in warnings.py (and test_warnings.py) - its sufficiently small that there is no point having a new module IMO. And the sole remaining functionality issue is getting the interactions with ignore_deprecations_here lined up.

The key thing is that we have two fixtures here - Deprecations and IgnoreDeprecationsHere, both of which should nest cleanly.

cleanly meaning:

• state changes made within the fixture don't leak outside the fixture
• we need multiple deprecations to nest additively
• ignoredeprecationshere should ignore every deprecation while active

---

Review status: 0 of 6 files reviewed at latest revision, 17 unresolved discussions.

---

fixtures/_fixtures/deprecations.py, line 19 [r2] (raw file):
Oh ugh. Uh, fixtures._fixtures.warnings - yes, lets rename it (but it can be a separate patch, so this is fine for now).

---

fixtures/_fixtures/deprecations.py, line 44 [r2] (raw file):
Oh neat, cool.

---

fixtures/_fixtures/deprecations.py, line 66 [r3] (raw file):
Needs an s here I think.

---

fixtures/_fixtures/deprecations.py, line 71 [r3] (raw file):
plural here as well (call deprecated function), or perhaps 'call deprecated function(s)'

---

fixtures/_fixtures/deprecations.py, line 87 [r3] (raw file):
The doc says expect, the method is called ignore. Thats a bit confusing :)

---

fixtures/tests/_fixtures/test_deprecations.py, line 59 [r2] (raw file):
Thanks, thats great.

---

fixtures/tests/_fixtures/test_deprecations.py, line 28 [r3] (raw file):
So this still has the dependency on how python was invoked. I was proposing instead something like:

filters = warnings.filters[:]
self.addCleanup(operator.setitem, warningsfilters, slice(None, None), filters)
warnings.resetwarnings()

Either as a fixture itself, or in setUp to apply it to all tests.

---

fixtures/tests/_fixtures/test_deprecations.py, line 40 [r3] (raw file):
rather than MODULE, I think that name is clearer - less knowledge-at-a-distance.

---

fixtures/tests/_fixtures/test_deprecations.py, line 47 [r3] (raw file):
I mentioned this before - I think this needs to really save the state and restore it - as it stands its resetting the fixture, which loses state.

for instance:

useFixture(..)
with ignore_deprecations_here:
  warnings.filter.... some thing here
   ...
do_a_thing_that_should_be_filtered()

will fail today, I believe - and its not obvious that it will.

More seriously, I'm fairly sure that

useFixture(..)
warnings.filter.... some thing here
with ignore_deprecations_here:
   ...
do_a_thing_that_should_be_filtered()

will also fail: - because the cleanup called in ignore_deprecations_here() will wipe it out: and this is even less obvious.

So - what I'm proposing is that ignore_deprecations_here should not exit-and-enter: it should pop and insert the single filter that matters.

---

Comments from Reviewable