Code

Opened 16 months ago

Closed 15 months ago

Last modified 14 months ago

#19546 closed Cleanup/optimization (fixed)

Add logging configuration to test_sqlite.py

Reported by: bak1an Owned by: nobody
Component: Testing framework Version: master
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I am not able to see deprecation warnings while running tests with python 2.7. Adding -Wd and -Wall parameters has no effect either.

And only after adding py.warnings logger i finally got my warnings. So i'm proposing to add this logger into 'default' django test config like that.

It may help to prevent commits like this one.

Attachments (2)

19546.diff (930 bytes) - added by claudep 16 months ago.
19546-2.diff (5.9 KB) - added by ptone 16 months ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 16 months ago by bak1an

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set

hm, looks like some logging related tests are failing with this patch.

comment:2 Changed 16 months ago by bak1an

with this patches only one test failed.

comment:3 Changed 16 months ago by claudep

  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

I do think also that this is a serious problem if DeprecationWarning are not shown when running tests, whether for Django itself or for any Django app.
I'd go even further and propose to modify the DjangoTestSuiteRunner so as DeprecationWarnings are on by default (they can still be hidden when running test with verbosity=0).

Changed 16 months ago by claudep

comment:4 Changed 16 months ago by bak1an

@claudep you are right, they (warnings) need to be shown independently of current settings and loggers.

And your patch looks like RFC.

Version 0, edited 16 months ago by bak1an (next)

comment:5 Changed 16 months ago by aaugustin

Is this related to #18985 and [44046e8a38]?

If it is, is it enough to fix the problem in the test suite?

comment:6 Changed 16 months ago by claudep

Yes it is related. In #18985, we fixed the issue when running the development server and DEBUG=True. But tests are generally running with DEBUG=False, hence the warnings are filtered out by the standard logging configuration.

comment:7 Changed 16 months ago by aaugustin

  • Needs tests set

Makes sense. As far as I can tell, the patch just needs a test.

Changed 16 months ago by ptone

comment:8 Changed 16 months ago by ptone

I've attached a revised patch.

The setup for the logging tests now has a convoluted setup to NOT show the test deprecation during normal test runs. Because the warning and logging state is global, it is hard to test that our expected situation of capturing deprecations is working as expected, and at the same time show deprecations in tests, without polluting the test runner output every time with this test - so the setup saves and restores all streams.

The test runner takes some extreme steps to restore global state after running - this could be going too far for what is likely to be a single process running once?

The test for this patch uses machinery with a lot of overhead and takes between 1 and 2 seconds to run on my machine :(

I'm going to hold off on committing this for at least part of a day in case anyone can offer suggestions to the above shortcomings.

comment:9 follow-up: Changed 16 months ago by aaugustin

Would it be easier to capture stderr entirely?

from StringIO import StringIO     # needs Python 3 compatible import
import sys

new_stderr = StringIO()
old_stderr, sys.stderr = sys.stderr, new_stderr
try:
    warnings.warn('Foo Deprecated', DeprecationWarning)
    self.assertIn('Foo Deprecated', output.getvalue())
finally:
    sys.stderr = old_stderr

comment:10 in reply to: ↑ 9 Changed 16 months ago by ptone

Replying to aaugustin:

Would it be easier to capture stderr entirely?

I had tried that with a local instance of DjangoTestSuiteRunner - but invoking it's run_tests method inside a test proved too thorny and I went the admin scripts route.

unittest does something funky to capture/store the original stderr stream at least for warnings before running the test, even before the setUp method - I find this surprising, but can't explain why this strategy doesn't work

so the straightforward way to attempt this isn't working, this gist represents my increasingly verbose attempt - which does not work:

https://gist.github.com/4444864

I know some would say 1-2 seconds isn't that long - but I still find that a sort of painful addition.

comment:11 Changed 15 months ago by ptone

so in trying to wrap this up - I've simplified the patch to be a little less engineered and just assume that stdlib global state is likely as it would be set by LazySettings

https://github.com/ptone/django/compare/ticket/19546

but am getting a fair of unwanted deprecation warning noise now when running the tests under py3

https://gist.github.com/6d384b4110fd916d1fd2

The source and solution to these warnings (and why they are py3 only) I haven't yet had time to fully investigate.

comment:12 Changed 15 months ago by claudep

  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

This looks good to me. I just fixed some of the deprecation warnings. I think this is a good sign that your patch is successful :-)

comment:13 Changed 15 months ago by Preston Holmes <preston@…>

  • Resolution set to fixed
  • Status changed from new to closed

In cfa70d0c94a43d94e3ee48db87f2b88c29a862e1:

Fixed #19546 - ensure that deprecation warnings are shown during tests

refs #18985

comment:14 Changed 15 months ago by Preston Holmes <preston@…>

In af8e858c1596623ffcccedc57433a009455bc314:

[1.5.x] Fixed #19546 - ensure that deprecation warnings are shown during tests

refs #18985

comment:15 Changed 15 months ago by Claude Paroz <claude@…>

In e6949373b0e4fcacde4cc269b647f1e1be8cade9:

Skipped deprecation warning test on Python 2.6

Refs #19546. On Python 2.6, DeprecationWarnings are visible by
default.

comment:16 Changed 15 months ago by Claude Paroz <claude@…>

In 785ec24720b3d7b38cf683f298b730c0c4014ac9:

[1.5.x] Skipped deprecation warning test on Python 2.6

Refs #19546. On Python 2.6, DeprecationWarnings are visible by
default.
Backport of e6949373b from master.

comment:17 Changed 15 months ago by Florian Apolloner <florian@…>

In 1d3368b587c786128d1cf0b6f172f35e03269ecc:

[1.5.X] Fixed lockups in jenkins, refs #19546.

Backport of ce580dd8ea04237cfe34cd02df0b8944a5345f4f from master.

comment:18 Changed 15 months ago by Florian Apolloner <florian@…>

In ce580dd8ea04237cfe34cd02df0b8944a5345f4f:

Fixed lockups in jenkins, refs #19546.

comment:19 Changed 14 months ago by Preston Holmes <preston@…>

In af8e858c1596623ffcccedc57433a009455bc314:

[1.5.x] Fixed #19546 - ensure that deprecation warnings are shown during tests

refs #18985

comment:20 Changed 14 months ago by Claude Paroz <claude@…>

In 785ec24720b3d7b38cf683f298b730c0c4014ac9:

[1.5.x] Skipped deprecation warning test on Python 2.6

Refs #19546. On Python 2.6, DeprecationWarnings are visible by
default.
Backport of e6949373b from master.

comment:21 Changed 14 months ago by Florian Apolloner <florian@…>

In 1d3368b587c786128d1cf0b6f172f35e03269ecc:

[1.5.X] Fixed lockups in jenkins, refs #19546.

Backport of ce580dd8ea04237cfe34cd02df0b8944a5345f4f from master.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.