-
Notifications
You must be signed in to change notification settings - Fork 51
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
Refactor watchdog test case (New) #1125
base: main
Are you sure you want to change the base?
Conversation
d2c2fa7
to
565ad2f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1125 +/- ##
==========================================
- Coverage 43.21% 43.12% -0.09%
==========================================
Files 356 352 -4
Lines 38662 38543 -119
Branches 6561 6555 -6
==========================================
- Hits 16706 16620 -86
+ Misses 21293 21257 -36
- Partials 663 666 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
671efd3
to
a44a16d
Compare
9c710a8
to
6fbcf94
Compare
The |
Thank you. Removed! |
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.
Please split the PR into two (pxus are a followup to python)!
Gave some inline feedback here and there
Latest submission: https://certification.canonical.com/hardware/202306-31658/submission/367954/ |
I will rewrite the unittest when this is being considered an approved PR. |
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.
Some comments here and there, only major thing is the detect function that can't work if there is more than 1 watchdog. Its unclear to me what to do in this situation, especially if there is more than 1 watchdog kind. I have proposed a patch to the function, but we should consider what to actually do in this situation, because this may not be what we want
- one prerequisite: Set watchdog config variable before starting the tests
Co-authored-by: Massimiliano <[email protected]>
Co-authored-by: Massimiliano <[email protected]>
Co-authored-by: Massimiliano <[email protected]>
Co-authored-by: Massimiliano <[email protected]>
Co-authored-by: Massimiliano <[email protected]>
760d798
to
10601ec
Compare
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.
@Hook25 Please see my comment.
imports: from com.canonical.plainbox import manifest | ||
requires: | ||
manifest.has_hardware_watchdog == 'True' | ||
environ: WATCHDOG_TYPE WATCHDOG_IDENTITY |
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.
@Hook25 I notice that in my jobs I require some environment variables like WATCHDOG_TYPE and
WATCHDOG_IDENTITY, however, the machines in the sru pool has not been configured those variables, as a result, it may break the sru testing.
Do you have any idea how we can prevent from this?
Description
After refactoring watchdog test cases, there are several benefits and flexibility.
The only prerequisite is to set up the environment variable before starting the test.
Resolved issues
N/A
Documentation
N/A
Tests