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

filter_record_tuples() implementation #54

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

TBeijen
Copy link

@TBeijen TBeijen commented Jan 2, 2017

I find myself often using the following construct when asserting if a certain log message has been recorded:

import logging

def test_things(caplog):
    # things
    assert list(filter(lambda r: r[0] == 'my.module' and r[1] == logging.WARNING, caplog.record_tuples))

Some of the drawbacks:

  • Lot of repetition of constructs not part of the test, e.g. the need to wrap the filter result in list to be able to assert the length
  • Need to import logging merely to obtain the log level

I propose an extension to caplog that looks like this:

def test_things(caplog):
    # things
    assert caplog.filter_record_tuples('my.module', caplog.WARNING)

Please let me know your thoughts. Do I need to create an issue for this proposal?

Edit:

  • Changed caplogs.levels.WARNING into caplog.WARNING
  • Removed references to deprecated 'WARN' (Will not include it in levels exposed)
  • Removed 'Work in Progress' from PR

README.rst Outdated
To aid in common test scenarios, caplog exposes logging levels under
``caplog.levels`` to prevent the need to import the ``logging`` module in tests.
Furthermore ``filter_record_tuples`` can be used to easily filter log messages of
a particular logger. This ise specially useful for testing composite systems where
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ise specially - a typo?

@abusalimov
Copy link
Collaborator

@TBeijen Thanks for submitting this and happy New Year! 🎄

I like the idea in general, and I though about adding somewhat similar too. And if we finally agree on introducing this, I would also suggest to add a caplog.filter_records() method too, to be consistent with the caplog.records and caplog.record_tuples properties.

@TBeijen
Copy link
Author

TBeijen commented Jan 3, 2017

@abusalimov Happy NY too!

Thanks for the swift reply. Implementing caplog.filter_records() had crossed my mind also. I doubted a bit as caplog.record_tuples seems intended for use in test asserts and wasn't sure of typical use cases of caplog.records. Will add.

On a sidenote: I'm now removing the .levels part so it will be caplog.WARNING etc.. The levels part didn't seem to add much and logging has it levels exposed as direct attributes also so this might be more intuitive.

@TBeijen TBeijen changed the title WIP: filter_record_tuples() implementation filter_record_tuples() implementation Jan 3, 2017
@TBeijen
Copy link
Author

TBeijen commented Jan 3, 2017

Done. See Readme change for features implemented.

I've disabled travis builds on pypy3, see (travis-ci/travis-ci#6277 (comment))

I must say that after all I'm not so sure of the need to add filter_records as well. I couldn't come up with a use case where it would have any advantages over filter_record_tuples so I'd rather remove it (keeping things simple, YAGNI).

README.rst Outdated
assert not any([r.levelno >= caplog.ERROR for r in caplog.filter_records('components.a')])

assert caplog.filter_record_tuples('components.a', caplog.levels.INFO, 'foo')
assert caplog.filter_record_tuples('components.b', caplog.levels.INFO, re.compile(r'foo\s.+'))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it caplog.levels.INFO or just caplog.INFO after all?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overlooked that. :/ It's caplog.INFO. Fixed now.

@@ -16,6 +16,9 @@ class LogCaptureFixture(object):
def __init__(self, item):
"""Creates a new funcarg."""
self._item = item
# set logging levels to facilitate use without needing to import logging
for level in ['CRITICAL', 'ERROR', 'WARNING', 'INFO', 'DEBUG', 'NOTSET']:
setattr(self, level, getattr(logging, level))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this is pretty straightforward and simple to understand, I'm not quite sure that hard-coding the exact levels is the best choice here. Users may define their own log levels, and AFAIR the logging module explicitly allows that (please correct me if I'm wrong).

What about dispatching every UPPER-case property access to the logging module? Something like this:

    __getattr__(self, name):
        if name == name.upper():
            return getattr(logging, name)
        return getattr(super(...), name)

Copy link
Author

@TBeijen TBeijen Jan 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to consider custom log levels. I did some digging (and by that learning). The logging module indeed allows registering log levels via logging.addLevelName(lvl, name, but... levels are registered in a 'protected' dict and not exposed as attributes. A level is just an integer and the purpose of registering of levels by label is only to have that label appear in log records.

There is a logging.getLevelName(lvl) method but not a getAllLevels().

Commands below summarize how custom levels work. Note that in python3 there's no _levelNames but instead there are _levelToName and _nameToLevel.

>>> import sys
>>> sys.version_info
sys.version_info(major=2, minor=7, micro=12, releaselevel='final', serial=0)
>>> import logging
>>> logging.basicConfig(format='%(levelname)s:%(message)s')
>>> logging.addLevelName(45, 'custom')
>>> logging.getLevelName(45)
'custom'
>>> logging.log(30, 'message')
WARNING:message
>>> logging.log(45, 'message')
custom:message
>>> logging.log(46, 'message')
Level 46:message
>>> logging.WARNING
30
>>> logging.custom
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'module' object has no attribute 'custom'
>>> logging._levelNames
{0: 'NOTSET', 10: 'DEBUG', 'WARN': 30, 20: 'INFO', 'ERROR': 40, 'DEBUG': 10, 30: 'WARNING', 'INFO': 20, 'WARNING': 30, 40: 'ERROR', 45: 'custom', 'custom': 45, 50: 'CRITICAL', 'CRITICAL': 50, 'NOTSET': 0}

Based on this I'd say facilitating custom levels is far-fetched. As levels are merely numbers I'll add a test for a custom numeric level.

Leaves the actual implementation. On a first glance it indeed doesn't look like very good practice to list these levels, on the other hand, it is very explicit and the levels aren't really subject to change.

@abusalimov Let me know if you still want the implementation changed. My opinion is that changing it doesn't improve, but it's not my project. ;)

Reference:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TBeijen Sure, I see. But still, what do you think about the proposed solution?

    def __getattr__(self, name):
        if name == name.upper():
            return getattr(logging, name)
        return getattr(super(...), name)

It seems to solve the issue in question in a generic way, without the need to know any of existing levels.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generic of course is also a good thing...
See update, slightly different check but otherwise the same as your proposal.

@TBeijen
Copy link
Author

TBeijen commented Apr 2, 2017

I notice it's still marked as 'changes requested'. I think 385c440 fixes that. Anything I can do to move this PR forward?

@abusalimov
Copy link
Collaborator

abusalimov commented Apr 3, 2017

@TBeijen Please notice my comments above/ Sorry, they have been left pending for all these months. =\

if name.isupper() and name.isalpha():
# lookup names matching level pattern in logging module
return getattr(logging, name)
return getattr(self, name)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, there should be super(...) in place on self, otherwise we end up with infinite recursion, don't we? Please correct me if I'm wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is still there. caplog.unknown_attr -> infinite recursion.

setattr(self, level, getattr(logging, level))

def __getattr__(self, name):
if name.isupper() and name.isalpha():
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the isalpha() check for?


def test_levels_error(caplog):
with pytest.raises(AttributeError):
getattr(caplog, 'UNKNOWNLEVEL')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, also add a check for lower case attributes.
Plus: 'LEVEL_WITH_UNDERSCORES', LEVEL42
Thanks

@TBeijen
Copy link
Author

TBeijen commented Apr 8, 2017

N/p that it takes some time, life can be busy.

Recursion was indeed not handled well. Fixed implementation and added a attribute lookup that will not be retrieved from logging module. (Sorry for polluting the history, should have run tox locally. Python 2.6 coding has been ages ago)

The combination of .isalpha() and .isupper() basically allows access to all log levels, but not to other attributes that don't fall into that pattern, like ._STYLES or .BASIC_FORMAT.

Custom loglevels, added for example via logging.setLevelName(13, 'custom'), are stored in logging._levelNames (python2) or logging._nameToLevel and logging._levelToName (python3).

The logging module itself uses .getLevelName() as described here to get a textual representation for a level. Illustrated by doing logging.getLevelName(123456) which returns Level 123456.

I would not go down the rabbit hole of looking up custom log levels. It's discouraged by python and adds complexity. If people insist on custom log levels they can always import logging itself in the tests.

Looking back at previous commits I actually think the explicit list of levels was better (because: explicit) than the isalpha(), isupper() combo (or lvl == lvl.upper() for that matter). Those levels haven't changed ever and are based on good practices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants