Skip to content
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

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

LiaoU3
Copy link
Contributor

@LiaoU3 LiaoU3 commented Mar 29, 2024

Description

After refactoring watchdog test cases, there are several benefits and flexibility.

  1. Fit all classic and core, oem or stock image.
  2. Automatically set watchdog timeout

The only prerequisite is to set up the environment variable before starting the test.

  • WATCHDOG_TYPE=wdat_wdt
  • WATCHDOG_IDENTITY=wdat_wdt

Resolved issues

N/A

Documentation

N/A

Tests

@LiaoU3 LiaoU3 requested a review from baconYao March 29, 2024 06:40
@LiaoU3 LiaoU3 force-pushed the migrate_iotg_watchdog branch from d2c2fa7 to 565ad2f Compare April 1, 2024 02:25
Copy link

codecov bot commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.12%. Comparing base (2ee5cc4) to head (a44a16d).
Report is 65 commits behind head on main.

Current head a44a16d differs from pull request most recent head 760d798

Please upload reports for the commit 760d798 to get more accurate results.

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     
Flag Coverage Δ
provider-base 16.12% <100.00%> (-0.18%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LiaoU3 LiaoU3 force-pushed the migrate_iotg_watchdog branch from 671efd3 to a44a16d Compare April 1, 2024 05:47
@LiaoU3 LiaoU3 marked this pull request as ready for review April 1, 2024 05:50
@LiaoU3 LiaoU3 closed this Apr 2, 2024
@LiaoU3 LiaoU3 deleted the migrate_iotg_watchdog branch April 2, 2024 08:37
@LiaoU3 LiaoU3 restored the migrate_iotg_watchdog branch April 2, 2024 09:02
@LiaoU3 LiaoU3 reopened this Apr 2, 2024
@LiaoU3 LiaoU3 requested a review from a team as a code owner April 17, 2024 07:25
@LiaoU3 LiaoU3 removed request for a team and baconYao April 17, 2024 07:26
@LiaoU3 LiaoU3 closed this Apr 17, 2024
@LiaoU3 LiaoU3 force-pushed the migrate_iotg_watchdog branch from 9c710a8 to 6fbcf94 Compare April 17, 2024 07:57
@LiaoU3 LiaoU3 reopened this Apr 17, 2024
@fernando79513
Copy link
Collaborator

The watchdog_config_test.py seems to be defined twice, both in providers/base/bin and providers/base/tests

@LiaoU3
Copy link
Contributor Author

LiaoU3 commented Apr 19, 2024

The watchdog_config_test.py seems to be defined twice, both in providers/base/bin and providers/base/tests

Thank you. Removed!

Copy link
Collaborator

@Hook25 Hook25 left a 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

providers/base/units/watchdog/jobs.pxu Outdated Show resolved Hide resolved
providers/base/units/watchdog/jobs.pxu Outdated Show resolved Hide resolved
providers/base/units/watchdog/jobs.pxu Show resolved Hide resolved
providers/base/bin/watchdog_config_test.py Outdated Show resolved Hide resolved
providers/base/bin/watchdog_config_test.py Outdated Show resolved Hide resolved
@LiaoU3
Copy link
Contributor Author

LiaoU3 commented May 6, 2024

@LiaoU3
Copy link
Contributor Author

LiaoU3 commented May 6, 2024

I will rewrite the unittest when this is being considered an approved PR.

@LiaoU3 LiaoU3 requested a review from Hook25 May 6, 2024 08:58
Copy link
Collaborator

@Hook25 Hook25 left a 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

providers/base/bin/watchdog_config_test.py Show resolved Hide resolved
providers/base/bin/watchdog_config_test.py Outdated Show resolved Hide resolved
providers/base/bin/watchdog_config_test.py Outdated Show resolved Hide resolved
providers/base/units/watchdog/jobs.pxu Outdated Show resolved Hide resolved
providers/base/units/watchdog/jobs.pxu Outdated Show resolved Hide resolved
providers/base/units/watchdog/jobs.pxu Outdated Show resolved Hide resolved
providers/base/units/watchdog/jobs.pxu Show resolved Hide resolved
providers/base/units/watchdog/jobs.pxu Show resolved Hide resolved
providers/base/units/watchdog/jobs.pxu Show resolved Hide resolved
@LiaoU3 LiaoU3 requested a review from Hook25 June 21, 2024 04:05
Hook25
Hook25 previously approved these changes Jun 24, 2024
@fernando79513 fernando79513 added the waiting-for-changes The review has been completed but the PR is waiting for changes from the author label Jul 16, 2024
@zongminl zongminl marked this pull request as draft July 19, 2024 03:43
LiaoU3 added 3 commits August 6, 2024 14:38
 - one prerequisite: Set watchdog config variable before starting the tests
Copy link
Contributor Author

@LiaoU3 LiaoU3 left a 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
Copy link
Contributor Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-changes The review has been completed but the PR is waiting for changes from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants