Add suspend stats related check (Bugfix) #1700
Conversation
1. test case is_suspend_success will be merged into suspend_advanced_auto 2. is_device_supend_success is used to check any fail
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1700 +/- ##
==========================================
+ Coverage 49.01% 49.06% +0.05%
==========================================
Files 372 373 +1
Lines 40348 40392 +44
Branches 6817 6827 +10
==========================================
+ Hits 19777 19819 +42
- Misses 19849 19851 +2
Partials 722 722
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
hanhsuan
changed the title
There was a problem hiding this comment.
Well done, I really like what I see and I wouldn't be against merging (the half that checks we did actually suspend) in suspend advanced auto.
I have 2 big worries:
- Is this supported on any kernel we have to support? Even xenial? Can you check? If not, what do we do about it?
- Does this work from a strict snap? Can you check?
| for root, dirs, files in os.walk(search_directory): | ||
| for file_name in files: | ||
| file_path = os.path.join(root, file_name) | ||
| with open( | ||
| file_path, "r", encoding="utf-8", errors="ignore" | ||
| ) as file: | ||
| content[file_name] = file.read().splitlines()[0] | ||
| return content |
There was a problem hiding this comment.
Slightly easier to read imo
| for root, dirs, files in os.walk(search_directory): | |
| for file_name in files: | |
| file_path = os.path.join(root, file_name) | |
| with open( | |
| file_path, "r", encoding="utf-8", errors="ignore" | |
| ) as file: | |
| content[file_name] = file.read().splitlines()[0] | |
| return content | |
| search_directory = Path(search_directory) | |
| for p in filter(lambda x: x.is_file(), search_directory.iterdir()): | |
| content[p.name], *_ = p.read_text().splitlines(1) | |
| return content |
| for c in self.contents: | ||
| print("{}:{}".format(c, self.contents[c])) |
There was a problem hiding this comment.
| for c in self.contents: | |
| print("{}:{}".format(c, self.contents[c])) | |
| for c, v in self.contents.items(): | |
| print("{}:{}".format(c, v)) |
| return ( | ||
| self.contents["success"] != "0" | ||
| and self.contents["failed_prepare"] == "0" | ||
| and self.contents["failed_suspend"] == "0" | ||
| and self.contents["failed_resume"] == "0" | ||
| ) |
There was a problem hiding this comment.
Big questoion is, why are we comparing these values as strings? Is it even possible that these are not valid integers? if that happens I suppose its a big deal, for this reason I've also suggested to remove the utf8/ignore errors above, if sysfs is broken we want to know
Another big question is what do we do if one or more of these files is missing? is it possible? if it is, when does it happen? should we have a default for those situations?
| parser_any.add_argument( | ||
| "-p", | ||
| "--print", | ||
| dest="print", | ||
| action="store_true", | ||
| help="Print content", | ||
| ) |
There was a problem hiding this comment.
Given how this will be used/how this works, I would like it to always print, remove this parameter
| parser_valid.add_argument( | ||
| "-p", | ||
| "--print", | ||
| dest="print", | ||
| action="store_true", | ||
| help="Print content", | ||
| ) |
| subparsers.required = True | ||
|
|
||
| # Add parser for validating the system is after suspend or not | ||
| parser_valid = subparsers.add_parser( |
There was a problem hiding this comment.
This is a bit of a weird usage of subparsers, I would use an argument (check_kind or something) that is a choice between:
- after_suspend: Check that the system is after suspend or fail
- any_failure: Check that no suspend failure is reported in sysfs or fail
| def main(self): | ||
| args = self.parse_args() | ||
| if args.type == "valid": | ||
| if args.print: | ||
| self.print_all_content() | ||
| if not self.is_after_suspend(): | ||
| raise SystemExit("System is not under after suspend status") | ||
| elif args.type == "any": | ||
| if args.print: | ||
| self.print_all_content() | ||
| if self.is_any_failed(): | ||
| raise SystemExit( | ||
| "There are [{}] failed".format(self.contents["fail"]) | ||
| ) |
There was a problem hiding this comment.
If the sysfs nodes may not be there please check here and... lets discuss what to do
Consider the following:
| def main(self): | |
| args = self.parse_args() | |
| if args.type == "valid": | |
| if args.print: | |
| self.print_all_content() | |
| if not self.is_after_suspend(): | |
| raise SystemExit("System is not under after suspend status") | |
| elif args.type == "any": | |
| if args.print: | |
| self.print_all_content() | |
| if self.is_any_failed(): | |
| raise SystemExit( | |
| "There are [{}] failed".format(self.contents["fail"]) | |
| ) | |
| def main(self): | |
| args = self.parse_args() | |
| self.print_all_content() | |
| if args.type == "valid": | |
| if not self.is_after_suspend(): | |
| raise SystemExit("System is not after suspend (didn't suspend even once)") | |
| print("System is after suspend") | |
| elif args.type == "any": | |
| if self.is_any_failed(): | |
| raise SystemExit( | |
| "There are [{}] suspend failures reported in sysfs".format(self.contents["fail"]) | |
| ) | |
| print("No suspend failure reported in sysfs") | |
| id: suspend/valid_suspend_status | ||
| plugin: shell | ||
| category_id: com.canonical.plainbox::suspend | ||
| depends: suspend/suspend_advanced_auto | ||
| estimated_duration: 5s | ||
| command: suspend_stats.py valid -p | ||
| summary: | ||
| Test this machine suspend status is success or not | ||
| description: | ||
| If the suspend is successed, the count in /sys/power/suspend_stats/success will be non zero | ||
| and the /sys/power/suspend_stats/failed_suspend will be zero |
There was a problem hiding this comment.
(removed -p in line with above, see if you like the new interface)
Slightly changed wording:
| id: suspend/valid_suspend_status | |
| plugin: shell | |
| category_id: com.canonical.plainbox::suspend | |
| depends: suspend/suspend_advanced_auto | |
| estimated_duration: 5s | |
| command: suspend_stats.py valid -p | |
| summary: | |
| Test this machine suspend status is success or not | |
| description: | |
| If the suspend is successed, the count in /sys/power/suspend_stats/success will be non zero | |
| and the /sys/power/suspend_stats/failed_suspend will be zero | |
| id: suspend/validate_suspend_status | |
| plugin: shell | |
| category_id: com.canonical.plainbox::suspend | |
| depends: suspend/suspend_advanced_auto | |
| estimated_duration: 5s | |
| command: suspend_stats.py after_suspend | |
| _summary: Tests that the machine suspended correctly | |
| _description: | |
| Query sysfs for the amount of times the system was suspended. We expect the | |
| count in /sys/power/suspend_stats/success to be non zero |
| id: suspend/any_fail_during_suspend | ||
| plugin: shell | ||
| category_id: com.canonical.plainbox::suspend | ||
| depends: suspend/suspend_advanced_auto | ||
| estimated_duration: 5s | ||
| command: suspend_stats.py any -p | ||
| summary: | ||
| Test all devices in this machine suspend status is failed or not | ||
| description: | ||
| If the suspend is successed, the count in /sys/power/suspend_stats/success will be non zero | ||
| and the /sys/power/suspend_stats/fail will be zero. If there is device failed, | ||
| /sys/power/suspend_stats/last_failed_dev will show the failed device |
There was a problem hiding this comment.
| id: suspend/any_fail_during_suspend | |
| plugin: shell | |
| category_id: com.canonical.plainbox::suspend | |
| depends: suspend/suspend_advanced_auto | |
| estimated_duration: 5s | |
| command: suspend_stats.py any -p | |
| summary: | |
| Test all devices in this machine suspend status is failed or not | |
| description: | |
| If the suspend is successed, the count in /sys/power/suspend_stats/success will be non zero | |
| and the /sys/power/suspend_stats/fail will be zero. If there is device failed, | |
| /sys/power/suspend_stats/last_failed_dev will show the failed device | |
| id: suspend/any_suspend_failure | |
| plugin: shell | |
| category_id: com.canonical.plainbox::suspend | |
| depends: suspend/suspend_advanced_auto | |
| estimated_duration: 5s | |
| command: suspend_stats.py any_failure | |
| summary: Tests if any device in this machine reports a suspend failure | |
| description: | |
| If the DUT suspended successfully, /sys/power/suspend_stats/fail will be zero. | |
| If a device failed /sys/power/suspend_stats/last_failed_dev will show the failed device |
| suspend/valid_suspend_status | ||
| suspend/any_fail_during_suspend |
There was a problem hiding this comment.
If you like the new ids:
| suspend/valid_suspend_status | |
| suspend/any_fail_during_suspend | |
| suspend/validate_suspend_status | |
| suspend/any_suspend_failure |
|
Adding this I dug up from the kernel docs: https://github.com/torvalds/linux/blob/69e858e0b8b2ea07759e995aa383e8780d9d140c/Documentation/ABI/testing/sysfs-power#L317C9-L317C40 If this means what I think it means, then we need a fallback mechanism |
Description
WARNING: This modifies com.canonical.certification::sru-server
According to the
suspend_advanced_autois the dependency for all after suspend jobs, it would be better to do more tests for this concept to fix #1579.Therefore, this PR is the first step to collect information by:
valid_suspend_statusis used to checksuccessshould be non zeroany_fail_during_supendis used to checkfailshould be zeroIf everything woks well,
valid_suspend_statuswill be merge intosuspend_advanced_autoin the future.Resolved issues
#1579
Documentation
Tests
succ 202001-27667
fail 202412-36131