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

Fail test run when QEMU fails too quickly #266

Merged
merged 1 commit into from
Feb 1, 2024
Merged

Conversation

kostyanf14
Copy link
Contributor

No description provided.

Copy link
Contributor

@akihikodaki akihikodaki left a comment

Choose a reason for hiding this comment

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

Use Process#success?.
Also, can't we just always raise an error when the exit status is not 0?

@kostyanf14
Copy link
Contributor Author

Also, can't we just always raise an error when the exit status is not 0?

No. QEMU can crash, but after a restart, HLK tests will continue working.
If QEMU crashes once during hours, more likely just one test will fail.
For example DISK Stress (LOGO) crashes QEMU when an IDE disk is provided.
This is bad, but all other results are present. As we run testing Windows insider preview
versions, we can see strange crashes.

@akihikodaki
Copy link
Contributor

akihikodaki commented Nov 2, 2023

What about only checking the elapsed time then?
Something certainly went wrong when the elapsed time was too short.
Also, use: Process.clock_gettime(Process::CLOCK_MONOTONIC)

Copy link
Contributor

@akihikodaki akihikodaki left a comment

Choose a reason for hiding this comment

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

I added comments for a few things I missed in the last review.

@@ -99,7 +113,7 @@ def run_vm
@machine.run_pre_start_commands

qemu = run_qemu(scope)
qemu.wait_no_fail
@fails_quickly = check_fails_too_quickly(qemu.wait_no_fail&.exitstatus)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you raise immediately after qemu = nil?
You can also @fails_quickly turn into a local variable by doing so. It is better to have fewer states (instance variables).

It is also better to use . instead of &.; AutoHCK::CmdRun::wait_no_fail returns nil if the process does not terminate, but such a situation should not happen since Process::WNOHANG is not passed to AutoHCK::CmdRun::wait_no_fail. If AutoHCK::CmdRun::wait_no_fail returns nil despite that, it only implies the program has a bug. Assuming the process either succeeded or failed is wrong in that case, and we should raise an error instead.

Comment on lines 96 to 98
end

if @first_fail_time.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
end
if @first_fail_time.nil?
elsif @first_fail_time.nil?

@first_fail_time = Process.clock_gettime(Process::CLOCK_MONOTONIC)
false
else
(Process.clock_gettime(Process::CLOCK_MONOTONIC) - @first_fail_time) <= 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(Process.clock_gettime(Process::CLOCK_MONOTONIC) - @first_fail_time) <= 10
Process.clock_gettime(Process::CLOCK_MONOTONIC) - @first_fail_time <= 10

@YanVugenfirer YanVugenfirer merged commit d714ecf into master Feb 1, 2024
5 checks passed
@YanVugenfirer YanVugenfirer deleted the fail-quickly branch February 1, 2024 13:05
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