Skip to content
This repository was archived by the owner on Oct 21, 2022. It is now read-only.

Add unit test for compatibility_checker.py#44

Merged
liyanhui1228 merged 8 commits into
GoogleCloudPlatform:masterfrom
liyanhui1228:unit_checker
Jul 16, 2018
Merged

Add unit test for compatibility_checker.py#44
liyanhui1228 merged 8 commits into
GoogleCloudPlatform:masterfrom
liyanhui1228:unit_checker

Conversation

@liyanhui1228
Copy link
Copy Markdown
Member

Closes #22

Copy link
Copy Markdown
Contributor

@brianquinlan brianquinlan left a comment

Choose a reason for hiding this comment

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

I think that there is too much Mock usage here for these tests to be convincing. I'd say:

  1. don't test check() or retrying_check()
  2. mock retrying_check() to test get_...(). Don't mock ThreadPoolExecutor. Or maybe make the executor a constructor parameter and pass in a fake e.g.

class FakeExecutor:
def map(self, *args): return map(*args)

@liyanhui1228
Copy link
Copy Markdown
Member Author

@brianquinlan Thanks for the suggestion! Updated to use FakeExecutor to test the functions, and mocked def retrying_checks.

patch_config = mock.patch(
'compatibility_lib.compatibility_checker.configs', mock_config)

patch_executor = mock.patch(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would make this an optional constructor argument for CompatibilityChecker.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

@liyanhui1228 liyanhui1228 merged commit 5927d04 into GoogleCloudPlatform:master Jul 16, 2018
@liyanhui1228 liyanhui1228 deleted the unit_checker branch July 16, 2018 21:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants