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

Default behavior for unit tests: Fail if Errors are being logged and not Asserted. #138

Closed
zolyfarkas opened this issue Sep 18, 2020 · 10 comments

Comments

@zolyfarkas
Copy link

This is similar in ideology with failing a unit test that throws an unexpected exception. (Not asserted)

I have implemented this in Java (and more), see detail at: http://www.spf4j.org/spf4j-slf4j-test/index.html

textfixtures has nice log assertion utilities, and together with a "throwAssertionExceptionOnErrorLog()" that will throw an assertion error if a unit test logs an error and it does not assert it(expect it) would be so awesome!

I have been using this behavior in the Java world for a few years and it highlighted and stopped lots of issues from being shipped to production. A side effect I have seen is that developer logging will be more correct.

I really miss this in the python world..

@cjw296
Copy link
Member

cjw296 commented Sep 20, 2020

Can you give some examples of what tests using this facility would look like?

@zolyfarkas
Copy link
Author

One way I can think of that would not be far from the current implementation would look like:
Ability to set the LogCapture utility is a "strict mode" (a parameter to LogCapture that defaults to false for backward compatible behavior, but we can change what it defaults to, via a global var, etc... ) so that the following:

with LogCapture() as l:
     logger.error('an error')
@log_capture()
def test_function(capture):
    logger.error('an error')

would fail with and Assertion error like "Unasserted Error message Logged...."

and the developer would need to "fix" the code \ with a check confirming that it is OK for that context to log an error:

with LogCapture() as l:
     logger.error('an error')
     l.check(
        ('root', 'ERROR', 'an error')
     )
@log_capture()
def test_function(capture):
    logger.error('an error')
    capture.check(
        ('root', 'ERROR', 'an error')
     )

what do you think?

@cjw296
Copy link
Member

cjw296 commented Sep 22, 2020

Which of the following would you expect to cause a test failure:

with LogCapture() as l:
     logger.error('an error')
with LogCapture(enforce_checking=True) as l:
     logger.debug('a message')
with LogCapture(enforce_checking=True) as l:
     logger.error('an error')
     l.check(
        ('root', 'ERROR', 'an error')
     )
with LogCapture(enforce_checking=True) as l:
     l.check()
     logger.error('a message')
with LogCapture(enforce_checking=True) as l:
     logger.error('message 1')
     logger.error('message 2')
     logger.error('message 3')
     l.check_present(
        ('root', 'ERROR', 'message 2'),
        ('root', 'ERROR', 'message 3'),
     )
with LogCapture(enforce_checking=True) as l:
     logger.error('message 1')
     logger.error('message 2')
     logger.error('message 3')
     assert ('root', 'ERROR', 'message 2') in l.actual()
with LogCapture(enforce_checking=True) as l:
     logger.error('message 1')
     logger.error('message 2')
     logger.error('message 3')
     l.actual()

@zolyfarkas
Copy link
Author

I would expect to see failure for:

1 if enforce_checking=True ; 4; 5; 6; 7.

Now, with 6 and 7 the implementation can be tricky, and we could decide the a call to actual() "checks all entries" and
in that case 6 and 7 will not fail.

However I think we can do more:

  1. I assume that we can store a flag (checked_flag) along each captured log entry to record if the log entry has been checked/asserted against (via check, check_present). This flag is what we will use to make sure all errors have been checked and throw and AssertionError if not.
  2. actual() will return a "view" of the captured logs. Any access to a log entry via this view we can consider as a "check" and updated the checked_flag accordingly. (for example a contains that return true will set the checked_flag=True for the respective log entry)

On a second thought I think the LogCapture class should implement the Sequence interface and one could directly do:

assert ('root', 'ERROR', 'message 2') in l

and we can use the same logic I proposed above and consider a contains return true as a valid "check" ...

let me know what you think

cheers.

@cjw296
Copy link
Member

cjw296 commented Sep 23, 2020

I'd be interested to see a PR with the cases above as tests, showing their output when they fail, but I suspect this may be more work that you expect.

@zolyfarkas
Copy link
Author

Will give it a try.

@zolyfarkas
Copy link
Author

@cjw296 what are the python version compatibility requirements?

@cjw296
Copy link
Member

cjw296 commented Sep 26, 2020

https://github.com/Simplistix/testfixtures/blob/9e96e7d20d5d6f71fa66372b8778c4b8e42d0bd0/setup.py#L34-L39

@zolyfarkas-fb
Copy link
Contributor

Hi, here is an attempt: #140

cjw296 added a commit that referenced this issue Oct 9, 2020
#138 Add default log check(assert) requirements for LogCapture
@cjw296
Copy link
Member

cjw296 commented Oct 9, 2020

#143

@cjw296 cjw296 closed this as completed Oct 9, 2020
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

No branches or pull requests

3 participants