-
Notifications
You must be signed in to change notification settings - Fork 276
[Issue 1848] Fixing parameterized test to mock out logger to fix log pollution #1929
Conversation
@blueandgold - I believe this is the last PR needed to fix #1848 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this here! A comment for a qq for my curiosity.
|
||
self.assertEqual(expect_delete_before_insert, delete_before_insert) | ||
with mock.patch('google.cloud.forseti.enforcer.gce_firewall_enforcer.LOGGER') as mock_logger: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this! Out of curiosity, why do we need to patch in the mock loggers this way (with with
), vs at the top of the method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question. I am no Python expert, so there may be a way. However, this method, unlike the others, uses @parameterized.parameterized.expand(
and I could not find a way to get the mock into those parameters. If you have a way, let me know and I'd be happy to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, let me play around with this, and see what is happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blueandgold - any thoughts here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blueandgold - friendly ping |
Fixed up and much cleaner now. I must have made a small syntax error when trying the simple mock. Thanks @ahoying ! |
No description provided.