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
kdump test #2904
kdump test #2904
Conversation
looks nice so far. But I don't fully understand what each test module is ensuring now. I recommend you put a full header into tests/toolchain/kdump.pm with a summary of what the module should test. Also we should avoid to fill up @mnowaksuse your review would be appreciated. |
lib/main_common.pm
Outdated
@@ -501,12 +501,13 @@ sub load_extra_tests() { | |||
# sysauth test scenarios run in the console | |||
loadtest "sysauth/sssd"; | |||
} | |||
# finished console test and back to desktop | |||
loadtest "console/consoletest_finish"; | |||
# kdump is not supported on aarch64, see BSC#990418 | |||
if (!check_var('ARCH', 'aarch64')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still enable them even for aarch64. Also the bug reference is wrong bsc#1037407. BSC#990418 is already closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, consoletest_finish is moved to the end. Because often it couldn't finish, and then the following tests will be skipped. Thanks for the hint about bug reference, I'll have a look.
Hi Oliver, kdump and crash test have the same set up steps, thus to avoid code duplication, the set up steps are extracted. Theoretically We could still merge them, because crash is based on kdump anyway. crash will do one more work than kdump, which is to analyse vmcore file generated by kdump. The problem now is that kdump is not stable, it behaves randomly on if it can be enabled or not. Besides that, the tests already function as expected on openSUSE Leap. At the moment I'm testing them on SLES and Tumbleweed, and it still needs to be improved. Thanks for your suggestions, I will try it out. |
I did an update of my WIP. |
02a3ff1
to
68acd17
Compare
I did an update of my WIP. Could you also have a look? @michalnowak mnowak |
@yxususe I will. Note that there's whitespace issue in Travis. |
Thanks @michalnowak, the whitespace issue is fixed now. |
lib/kdump_utils.pm
Outdated
script_run 'zypper ref'; | ||
zypper_call('-v in $(rpmquery kernel-default | sed "s/kernel-default/kernel-default-debuginfo/")'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you keep versioned installation of kernel-default-debuginfo package, please? We used to have problems with wrong kernel-default-debuginfo versions in SUT's repositories, which led to weird issues with crash
'es infrastructure.
lib/kdump_utils.pm
Outdated
# make sure kdump is enabled after reboot | ||
validate_script_output "yast2 kdump show 2>&1", sub { m/Kdump is enabled/ }, 90; | ||
#validate_script_output "yast2 kdump show 2>&1", sub { m/Kdump is enabled/ }, 90; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead "code". Remove it, perhaps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not perhaps, please remove :-)
lib/main_common.pm
Outdated
# finished console test and back to desktop | ||
loadtest "console/consoletest_finish"; | ||
# kdump is not supported on aarch64, see BSC#990418 | ||
# kdump is not supported on aarch64, see BSC#1037407 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make BSC lowercase, as it's the one we usually go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see inline comments and also provide verification runs for openSUSE and SLE.
lib/kdump_utils.pm
Outdated
@@ -1,32 +1,27 @@ | |||
# SUSE's openQA tests | |||
# | |||
# Copyright © 2016-2017 SUSE LLC | |||
# Copyright © 2017 SUSE LLC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please keep old as it refers to the content, not the file
lib/kdump_utils.pm
Outdated
# make sure kdump is enabled after reboot | ||
validate_script_output "yast2 kdump show 2>&1", sub { m/Kdump is enabled/ }, 90; | ||
#validate_script_output "yast2 kdump show 2>&1", sub { m/Kdump is enabled/ }, 90; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not perhaps, please remove :-)
tests/console/kdump_and_crash.pm
Outdated
# without any warranty. | ||
|
||
# Summary: Run 'crash' utility on a kernel memory dump | ||
# Maintainer: Michal Nowak <mnowak@suse.com> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to claim maintainership, or co-maintainership at least.
tests/console/kdump_and_crash.pm
Outdated
select_console 'root-console'; | ||
|
||
# soft failure ref bsc#1022064 if kdump could not be enabled | ||
return 1 unless kdump_is_active; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why to just exit the test with soft-fail? Rather die
instead as we spotted bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already a bug for it: https://bugzilla.suse.com/show_bug.cgi?id=957053. And when I run tests, often it fails because kdump doesn't work properly. Should we always let the test fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when I run tests, often it fails because kdump doesn't work properly
It looks to me like a definition of product bug. Then the test should always fail. Or I am missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the test always fails, what do we do with the ticket then? Resolved or rejected? For resolved we need at least one successful test run on o3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can always keep the ticket open in "Feedback" status limbo and wait for the product being fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Thanks for the hint.
I've done most modifications, except for die or soft failure part. But there's some issue with test run. I'm still testing it. |
lib/main_common.pm
Outdated
# finished console test and back to desktop | ||
loadtest "console/consoletest_finish"; | ||
# kdump is not supported on aarch64, see BSC#990418 | ||
# kdump is not supported on aarch64, see bsc#1037407 | ||
if (!check_var('ARCH', 'aarch64')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that you have kdump_is_active function, you can also enable this for aarch64, we can at least start testing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I'll do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@foursixnine what do you mean "we can start testing it". bsc#990418 is "RESOLVED WONTFIX" and bsc#1037407 is still open.
@yxususe in this PR please focus on one thing at a time. In this case I recommend to not enable testing on something that can not work. We can later on add it with a special switch to add a scenario to the test development group. We can hardly handle more "known" failures and rather should focus on having some more green jobs first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@okurz I mean that since the feature is already there, why don't test it?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course. But as long as the bug is still open we won't expect it to pass, no?
f4ea83e
to
b0652d9
Compare
I updated PR with Oliver's suggestion to leave enough time for crash: |
validation tests look fine so far. I recommend you update your commit message itself and the PR description. Please also revert the change reg. enabling the test for aarch64. |
The commit message is updated. Change referring aarch64 is reverted. |
No description provided.