add a manifest for cpufreq scaling test (Bugfix)#2425
Conversation
|
LGTM~ |
|
also should we update checkbox-provider-ce-oem/units/cpu/jobs.pxu to use this manifest? |
7468d5c to
827dc0c
Compare
fix black issue
|
this PR will change the CPU governor test as following scenario
|
revised the cpu scaling test classes
fix black8
fix format and compatible with py35
74b7a8a to
6b22300
Compare
update test job and scripts
fix coding style check issue
updated job requires
fix for no available cpu policy
tomli380576
left a comment
There was a problem hiding this comment.
the comments are mostly refactoring suggestions, lmk if i misunderstood anything
| def test_governor(self) -> bool: | ||
| """ | ||
| Run the CPU Scaling Governor Test. | ||
|
|
||
| This function is a placeholder for running the CPU scaling governor | ||
| test. It should be implemented in the subclasses for specific | ||
| governors. | ||
|
|
||
| Returns: | ||
| bool: True if the test passes, False otherwise. | ||
| """ | ||
| raise NotImplementedError( | ||
| "This method should be implemented in subclass." | ||
| ) |
There was a problem hiding this comment.
I think the more idiomatic way to do this is with an abstract base class:
-class CPUScalingTest
+class CPUScalingTest(abc.ABC)then at the method definition, decorate it:
@abc.abstractmethod
def test_governor(self) -> bool:
# no need to write anything in the method body
passthis can also prevent the base class from being constructed altogether.
| _registry = {} | ||
|
|
||
| @classmethod | ||
| def register(cls, key): | ||
| """Register test classes by governor name.""" | ||
|
|
||
| def decorator(test_cls): | ||
| cls._registry[key] = test_cls | ||
| return test_cls | ||
|
|
||
| return decorator |
There was a problem hiding this comment.
optional: i think it's less convoluted to just use a dict in main and put the class names in there:
# inside main
TEST_CLASSES = {
"powersave": PowersaveCPUScalingTest
"schedutil": SchedutilCPUScalingTest
# ...other ones
}
if args.governor not in TEST_CLASSES:
raise SystemExit("Unavailable governor: {}".format(args.governor))
# governor available, do the rest of main
for governor, test_class in TEST_CLASSES.items():
# do stuffthis can also eliminate the .create() method and allow main() to use normal class constructors
| class CPUScalingTest: | ||
| """A class for CPU scaling test operations.""" | ||
|
|
||
| test_description = "" |
There was a problem hiding this comment.
in the child classes this was changed to description, did you mean to use 2 different names?
If not, a more robust way to enforce the existence of test_description is to use an abstract property
# requires the parent class to be abstract
class CPUScalingTest(abc.ABC):
@property
@abc.abstractmethod
def test_description() -> str:
pass
class UserspaceCPUScalingTest(CPUScalingTest):
@property
def test_description() -> str:
return "some test description"
# access it like a normal property
t = UserspaceCPUScalingTest()
print(t.test_description)If the child class doesn't implement test_description, python will raise this:
TypeError: Can't instantiate abstract class UserspaceCPUScalingTest with abstract methods test_description
because not implementing the entire ABC's interface makes the child class implicitly abstract, thus cannot be constructed.
| print("cpb: {}".format(self.get_cpb(policy))) | ||
| print() | ||
| return True | ||
| self.policy = policy |
There was a problem hiding this comment.
is there a reason why we need to keep overwriting self.policy?
| @classmethod | ||
| def create(cls, governor, policy=0): | ||
| """ | ||
| Create a test instance by governor name. | ||
|
|
||
| Args: | ||
| governor (str): The name of the governor test to create. | ||
| policy (int): The CPU policy number to be used (default is 0). | ||
|
|
||
| Returns: | ||
| CPUScalingTest: An instance of the appropriate test class. | ||
|
|
||
| Raises: | ||
| ValueError: If the governor is not supported. | ||
| """ | ||
| test_class = cls._registry.get(governor.lower()) | ||
| if test_class is None: | ||
| raise ValueError( | ||
| "Governor '{}' not supported. Available: {}".format( | ||
| governor, ", ".join(sorted(cls._registry.keys())) | ||
| ) | ||
| ) | ||
| return test_class(policy=policy) |
There was a problem hiding this comment.
see the comment on 514
| probe_governor_module(args.governor) | ||
| if not getattr(test, "test_{}".format(args.governor))(): | ||
| logging.error( | ||
| "Governor '%s' is not supported by CPU policy%s", |
| except AttributeError: | ||
| logging.error("Given governor is not supported") | ||
| except ValueError as err: | ||
| logging.error(str(err)) |
There was a problem hiding this comment.
optional: depending on if you want to see the literal ValueError string, you could use repr
Description
add a manifest for cpufreq scaling test, as this feature is not supported on new i.MX9 SoC
Resolved issues
Documentation
Tests
test result when cpu policy is available
https://certification.canonical.com/hardware/202501-36212/submission/488805/
test result when cpu policy is unavailable