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

test: add a test that checking uptime when there is a invalid time in the log #1853

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yunmingyang
Copy link
Contributor

No description provided.

@yunmingyang
Copy link
Contributor Author

It smells like a bug that there will be a uptime with wheel user if change starting up time to 0.5, and no uptime with libvirt user if change starting up time to 0.5

@martinpitt
Copy link
Member

This test breaks (testBasicLibvirtUserUnprivileged) -- why would it show "years"?

@yunmingyang
Copy link
Contributor Author

@martinpitt I am not sure, but it seems that distanceToNow will calculate the difference between the current date and 0.5, even though 0.5 is not a valid date. Thus, the uptime will be shown as xx years. I guess that the reason why the testBasicLibvirtUserUnprivileged is breaked is libvirt user doesn't have enough privilege for accessing /var/log/libvirt/qemu.

@martinpitt
Copy link
Member

@yunmingyang I see -- but it feels a bit sad to put such a broken time there. Maybe something more realistic, like an empty date, or "0", or a non-numeric word?

@yunmingyang
Copy link
Contributor Author

For a non-numeric word, I think there should be already a case there, https://github.com/cockpit-project/cockpit-machines/blob/main/test/check-machines-lifecycle#L247. For 0 and empty date and 0, I think they are different scenarios, maybe we could add them both. But decimal should also be a scenario for checking uptime parsing robustness. Any comments? @martinpitt

@martinpitt
Copy link
Member

Mostly just that I don't quite like cementing a bad behaviour in a test case -- let's rather just fix the bug and ignore that invalid date?

@yunmingyang
Copy link
Contributor Author

All right

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.

2 participants