fix(logger): test generates logfile

[michaelbrewer]

Issue #, if available:

• Currently test_copy_config_to_ext_loggers_clean_old_handlers creates a file called "logfile"

Description of changes:

• Use NullHandler instead of FileHandler
• Update test codes
• Use isinstance over type

Checklist

• Meet tenets criteria
• Update tests
• Update docs
• PR title follows conventional commit semantics

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[michaelbrewer]

#937 (review)

@heitorlessa - So this is why :)

[codecov-commenter]

Codecov Report

Merging #971 (5ea99b3) into develop (7a6e49a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #971   +/-   ##
========================================
  Coverage    99.96%   99.96%           
========================================
  Files          119      119           
  Lines         5335     5335           
  Branches       607      607           
========================================
  Hits          5333     5333           
  Partials         2        2           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a6e49a...5ea99b3. Read the comment docs.

[michaelbrewer]

@mploski - I assume for the tests I can replace logging.FileHandler with logging.NullHandler, because the former is generating a file after the tests complete

[heitorlessa]

@mploski could you clarify what this test tests?

https://github.com/awslabs/aws-lambda-powertools-python/pull/971/files#diff-be72cf49c5b9ee9da9930ced7b33c6112dcc741221f3dfa0579b99e15fc8e8ffR145

I'm not entirely sure, perhaps what the THEN line should look like?

[mploski]

@mploski - I assume for the tests I can replace logging.FileHandler with logging.NullHandler, because the former is generating a file after the tests complete

thats correct @michaelbrewer we can use nullhandler or stream handler for example

[mploski]

@mploski could you clarify what this test tests?

https://github.com/awslabs/aws-lambda-powertools-python/pull/971/files#diff-be72cf49c5b9ee9da9930ced7b33c6112dcc741221f3dfa0579b99e15fc8e8ffR145

I'm not entirely sure, perhaps what the THEN line should look like?

test_copy_config_to_ext_loggers_clean_old_handlers test checks if external logger was reconfigured to use new handler and formatter and that old configured handler handler = logging.FileHandler("logfile") was cleaned

[michaelbrewer]

@mploski @heitorlessa - is this PR cool as it is? Or are there other changes needed?

[mploski]

@mploski @heitorlessa - is this PR cool as it is? Or are there other changes needed?

Looks good to me :)

[michaelbrewer]

@heitorlessa - anything outstanding?

[heitorlessa]

@heitorlessa - anything outstanding?

Michal comes back from PTO tomorrow and will make suggestions to fix his test comments to make the intent clearer - then we can merge (instead of creating another PR after merging this)

[mploski]

@michaelbrewer I reviewed test's comments I added some time ago for these tests and definitely I could have made them better :-) If you could tune them that would be great! Here are some suggestions, feel free to modify it :-)

test_copy_config_to_ext_loggers
Line 53 -> # GIVEN two external loggers and powertools logger initialized
Line 59 -> # WHEN configuration copied from powertools logger to ALL external loggers AND external loggers used
Line 66 -> # THEN all external loggers used Powertools handler, formatter and log level

test_copy_config_to_ext_loggers_include
Line 81 -> # WHEN configuration copied from powertools logger to INCLUDED external loggers AND our external logger used

Line 87 -> # THEN included external loggers used Powertools handler, formatter and log level.

test_copy_config_to_ext_loggers_wrong_include

Line 101 -> # WHEN configuration copied from powertools logger to INCLUDED NON EXISTING external loggers
Line 104 -> # THEN existing external logger is not modified

test_copy_config_to_ext_loggers_exclude
Line 113 -> # WHEN configuration copied from powertools logger to ALL BUT external logger
Line 116 -> # THEN external logger is not modified

test_copy_config_to_ext_loggers_include_exclude
Line 121 -> # GIVEN two external loggers and powertools logger initialized
Line 127 -> # WHEN configuration copied from powertools logger to INCLUDED external loggers AND external logger_1 is also in EXCLUDE list
Line 135 -> # THEN logger_1 is not modified and Logger_2 used Powertools handler, formatter and log level

test_copy_config_to_ext_loggers_clean_old_handlers
Line 152 -> # WHEN configuration copied from powertools logger to ALL external loggers
Line 155 -> # THEN old logger's handler removed and Powertools configuration used instead

test_copy_config_to_ext_loggers_custom_log_level
Line 167 -> # WHEN configuration copied from powertools logger to INCLUDED external logger AND external logger used with custom log_level
Line 174 -> # THEN external logger used Powertools handler, formatter and CUSTOM log level.

[michaelbrewer]

Cool @mploski all done.

[mploski]

Shouldn't it be something like WHEN configuration copied from powertools logger to INCLUDED NON EXISTING external loggers?

[mploski]

Looks great! Let's merge it