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

e2e: fix balloons test CPU frequencies #255

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

askervin
Copy link
Collaborator

@askervin askervin commented Feb 6, 2024

Use kHZ instead of MHz in minFreq and maxFreq as required in the real interface. Using wrong units in the tests did not fail because configuring CPU frequencies with any values does not really work in our test VMs. However, using wrong units in tests is misleading for anyone who tries to use similar configuration on bare metal.

Use kHZ instead of MHz in minFreq and maxFreq as required in the real
interface. Using wrong units in the tests did not fail because
configuring CPU frequencies with any values does not really work in
our test VMs. However, using wrong units in tests is misleading for
anyone who tries to use similar configuration on bare metal.

Signed-off-by: Antti Kervinen <[email protected]>
Copy link
Collaborator

@marquiz marquiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@marquiz marquiz merged commit 475d293 into containers:main Feb 6, 2024
2 checks passed
@klihub
Copy link
Collaborator

klihub commented Feb 7, 2024

@askervin This is a bit late and only tangentially related, but having the unit be allowed in those configuration frequency values would allow the user to specify them in a human-friendly syntax and reduce the possibility for mistakes. If we don't do that, then using parameter names like minFreqkHz and maxFreqkHz would make the settings' unit self-documenting/obvious.

@askervin
Copy link
Collaborator Author

askervin commented Feb 7, 2024

allow the user to specify them [units] in a human-friendly syntax

Definitely something we should do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants