-
Notifications
You must be signed in to change notification settings - Fork 163
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
[thermalctld] Refactor to allow for greater unit test coverage; Add more unit tests #157
[thermalctld] Refactor to allow for greater unit test coverage; Add more unit tests #157
Conversation
… testing purposes
@@ -180,7 +180,11 @@ def test_update_fan_with_exception(self): | |||
assert fan.get_status_led() == MockFan.STATUS_LED_COLOR_RED | |||
assert fan_updater.log_warning.call_count == 1 | |||
|
|||
fan_updater.log_warning.assert_called_with("Failed to update fan status - Failed to get speed") | |||
# TODO: Clean this up once we no longer need to support Python 2 | |||
if sys.version_info.major == 3: |
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.
How about we add a helper function like below:
def get_exception_message(msg):
if sys.version_info.major == 3:
return 'Exception(\'{},\')'.format(msg)
else:
return 'Exception(\'{}\')'.format(msg)
And we don't have to change everywhere when removing python2 support.
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.
Thank you for the suggestion. However, I believe this unit test code will not change much, if at all before we remove Python 2 support. And with things the way they are, all we have to do is delete lines. If we make the change you suggested, there will actually be a bit more work needed when removing Python 2 support. Therefore, I prefer we just leave things as-is for the time being.
@@ -23,7 +23,8 @@ | |||
tests_require=[ | |||
'mock>=2.0.0; python_version < "3.3"', | |||
'pytest', | |||
'pytest-cov' | |||
'pytest-cov', | |||
'sonic-platform-common' |
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.
@jleveque this dependency is causing sonic-net/sonic-buildimage#7143 sonic-buildimage to fail.
Do we need to remove it or add something ?
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.
PR to add dependency in sonic-buildimage here
Unit tests for thermalctld depend on sonic-platform-common as of sonic-net/sonic-platform-daemons#157
Unit tests for thermalctld depend on sonic-platform-common as of sonic-net/sonic-platform-daemons#157
Unit tests for thermalctld depend on sonic-platform-common as of sonic-net/sonic-platform-daemons#157
Unit tests for thermalctld depend on sonic-platform-common as of sonic-net/sonic-platform-daemons#157
- Make `_read_eeprom_specific_bytes` Python3 and Python2 compatible - Change a stray `self.eep_dict.iteritems()` call to `self.eep_dict.items()`
Description
Refactor thermalctld to reduce the amount of code in infinite loops, thus allowing us better unit test coverage
Refactor mock_platform.py such that it inherits from sonic-platform-common in order to ensure it is aligned with the current API definitions (this introduces a test-time dependency on the sonic-platform-common package)
Eliminate the need to check for a
THERMALCTLD_UNIT_TESTING
environment variable in daemon codeIncrease pytest verbosity to prevent truncation of error messages
Miscellaneous cleanup:
Increase overall unit test unit test coverage from 73% to 93%
Previous unit test coverage:
Unit test coverage with this patch:
Motivation and Context
As part of ongoing efforts to increase unit test coverage >= 90% across the SONiC codebase in order to prevent regressions
How Has This Been Tested?
The refactored thermalctld has been tested on a physical DUT and the unit test results can be examined via the Azure Pipelines check builds in this PR.