qa_automation: add new design for QA:HEAD tests #4132

cachen · 2017-12-27T10:44:05Z

-Split test workflow into 4 steps;
-Define functions to converting ctcs2 log to junit xml;
-Define functions to upload system logs;

Use for openQA to trigger ctcs2 testsuites from QA:HEAD; I would like to keep the original setup and tests in qa_automation module for not breaking the current test; and will remove them after make sure all the reset kernel/userspace tests work well.

for example:
BOOT_HDD_IMAGE=1
DESKTOP=textmode
HDD_1=<***>.qcow2
QA_HEAD_REPO=http://dist.nue.suse.com/ibs/QA:/Head/SLE-%VERSION%
QA_TESTSUITE=fs_stress
VIRTIO_CONSOLE=0

Related ticket: NO
Needles: NO
Verification run:
http://10.67.131.155/tests/384
http://10.67.131.155/tests/356

cachen · 2017-12-27T10:48:34Z

The last PR was closed by accident during rebase...reset...! Anyway, it's great to have this clean one, I have modified base on former comments :)
I will hand over to Sero to maintain, so I gave his name as maintainer.

ldevulder · 2017-12-27T16:15:01Z

Please use tidy --check to ensure that your code is clean :)
Or if you are in the process of developing, please add a WIP tag.

ldevulder

For me, other than these few comments, looks good!

ldevulder · 2017-12-28T08:36:27Z

lib/ctcs2_to_junit.pm

+            $fail_num += 1;
+        }
+    }
+    #my $count = $pass_nums + $fail_nums + $skip_nums;


Please avoid code in comment, if it's not necessary why do you need to keep it? Could be useful during development but not in a PR.
At least, add a comment to explain why you need to keep it.

ah, forgot to remove it.

ldevulder · 2017-12-28T08:38:19Z

lib/upload_system_log.pm

+
+our @EXPORT = qw(upload_system_logs);
+
+


Really a small thing, remove duplicate empty line ;-)

For perfect, it should be removed :)

ldevulder · 2017-12-28T08:45:22Z

tests/qa_automation/execute_test_run.pm

+
+sub run {
+    my ($self, %args) = @_;
+    my $timeout = $args{timeout} || 180 * 60;


Setting default timeout to 10800 (180*60) could be too high, as the default whole timeout is lesser than that (from what I remember it's 7200).
Please consider using MAX_JOB_TIME in your test configuration instead of setting a high timeout in the code, or be sure to always set the correct timeout when your test is called.

default timeout not enough for especially stress testing. 180 * 60 has verified in the current

Ok, but how 180*60 can be reached it you don't use MAX_JOB_TIME in your test configuration? As by default the global timeout is 7200 (I just verified it), so lesser than your default timeout.

Will see what another reviewer with merge access will say :-)

seems that %args could never be used in the case. and i can see your prove run completed in around 1 hour.
how about to set timeout like this:
my $timeout = get_var(MAX_JOB_TIME) || value<7200

The module will be called by a bunch of tests, each has difference time costing.
180 * 60 is to rewrite the default 7200, we have confirmed it's capability for our tests (in qa_run.pm), this time setup in here won't hurt openQA performance, and I want to avoid add too many 'var' configurations in test set :)
But @mitiao is correct, %args will never be called in this new modue, this should be removed!

Checked test list, few need specify timeout, use MAX_JOB_TIME configuration is accepted for flexibility :)
Done!

ldevulder · 2017-12-28T08:52:21Z

tests/qa_automation/prepare_qa_repo.pm

+    zypper_call("--no-gpg-check ar -f $qa_head_repo qa-ibs");
+    if ($qa_web_repo) {
+        zypper_call("--no-gpg-check ar -f $qa_web_repo qa-web");
+    }


Just a small remark here, you could replace your if (...) { zypper_call... } by zypper_call(...) if (...); ;-)

easy read for Perl none expert, I would like to keep :)

I'm neither a Perl expert and I personally find the other more readable :-) And a lot of openQA test codes uses it.
But it's ok for me ;-)

asmorodskyi · 2017-12-28T20:53:07Z

lib/upload_system_log.pm

+
+# Save the output of $cmd into $file and upload it
+sub system_status {
+    my $log = shift || "/tmp/system-status.log";


isn't first param will be always $self ? so your function will never be able to read passed param.

my ($self, $log) = @; $log //="/tmp/system-status.log";

this should work

it works, thanks a lot!

cachen · 2017-12-29T07:24:15Z

Thanks a lot for your review, Have updated according comments, please help to merge if no other critical issue :)

-Split test workflow into 4 steps; -Define functions to converting ctcs2 log to junit xml; -Define functions to upload system logs;

os-autoinst#4132 with title "qa_automation: add new design for QA:HEAD tests" added the use of the test variable QA_TESTSUITE whereas qam uses QA_TESTSET only. Checked with https://github.com/okurz/scripts/blob/master/openqa-db_query_last_use_of_module and found no QAM job running execute_test_run. https://openqa.suse.de/admin/test_suites shows that there are mainly QAM jobs using QA_TESTSET however some others as well. QA_TESTSUITE is not used for anything with a prefix like "qam". To me it looks like both are doing more or less the equivalent except that based on QA_TESTSET shows the output of individual test modules and also there is no module "execute_test_run" that would fail and is hard to label. The approach based on QA_TESTSET seems to provide much better visualization of test results using the more recent openQA feature to show "External results". Related progress issue: https://progress.opensuse.org/issues/44138

pevik · 2019-07-16T06:25:31Z

@cachen Hm, at a time, when a38f226 was merged, there was system_status() with similar functionality in tests/qa_automation/qa_run.pm. And this function is still there. It'd be nice if tests/qa_automation/qa_run.pm used lib/upload_system_log.pm.

cachen force-pushed the master branch 4 times, most recently from f2daa1a to ac4e10f Compare December 28, 2017 04:50

ldevulder requested changes Dec 28, 2017

View reviewed changes

asmorodskyi reviewed Dec 28, 2017

View reviewed changes

cachen force-pushed the master branch from 938758f to b730215 Compare December 29, 2017 06:06

qa_automation: add new design for QA:HEAD tests

a38f226

-Split test workflow into 4 steps; -Define functions to converting ctcs2 log to junit xml; -Define functions to upload system logs;

cachen force-pushed the master branch from 06515b4 to a38f226 Compare January 2, 2018 06:33

mitiao merged commit 1f60588 into os-autoinst:master Jan 2, 2018

okurz mentioned this pull request Mar 4, 2019

[WIP][DISCUSSION] Fully replace QA_TESTSUITE approach by QA_TESTSET #6955

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qa_automation: add new design for QA:HEAD tests #4132

qa_automation: add new design for QA:HEAD tests #4132

cachen commented Dec 27, 2017

cachen commented Dec 27, 2017

ldevulder commented Dec 27, 2017

ldevulder left a comment

ldevulder Dec 28, 2017

cachen Dec 29, 2017

ldevulder Dec 28, 2017

cachen Dec 29, 2017

ldevulder Dec 28, 2017

cachen Dec 29, 2017

ldevulder Dec 29, 2017

mitiao Jan 2, 2018

cachen Jan 2, 2018 •

edited

cachen Jan 2, 2018 •

edited

ldevulder Dec 28, 2017

cachen Dec 29, 2017

ldevulder Dec 29, 2017

asmorodskyi Dec 28, 2017 •

edited

cachen Dec 29, 2017 •

edited

cachen commented Dec 29, 2017

pevik commented Jul 16, 2019

qa_automation: add new design for QA:HEAD tests #4132

qa_automation: add new design for QA:HEAD tests #4132

Conversation

cachen commented Dec 27, 2017

cachen commented Dec 27, 2017

ldevulder commented Dec 27, 2017

ldevulder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cachen Jan 2, 2018 • edited

Choose a reason for hiding this comment

cachen Jan 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmorodskyi Dec 28, 2017 • edited

Choose a reason for hiding this comment

cachen Dec 29, 2017 • edited

Choose a reason for hiding this comment

cachen commented Dec 29, 2017

pevik commented Jul 16, 2019

cachen Jan 2, 2018 •

edited

cachen Jan 2, 2018 •

edited

asmorodskyi Dec 28, 2017 •

edited

cachen Dec 29, 2017 •

edited