Differential D650

explicitly check for DNF "system is not ready for upgrade"
ClosedPublic
Actions

Authored by adamwill on Nov 17 2015, 7:05 PM.

Details

Reviewers
garretraziel
jskladan
Commits
rOPENQATESTS3ecef54b51e7: explicitly check for DNF "system is not ready for upgrade"
Summary

Instead of sitting there waiting 6000 seconds twice, when DNF
explicitly tells us it failed, just die.

This is why we haven't been getting proper compose check reports
lately; the upgrade tests are failing, waiting 6000 seconds to
time out, then being cloned and tried again, waiting another 6000
seconds. This is just barely going beyond check-compose's 8 hour
wait limit, as it's some time before the upgrade tests even get
started (they're low in the priority list). We're still going to
have that problem if the tests fail any other way, but this at
least catches that case.

Test Plan

Run the upgrade tests and see that they fail quicker
(assuming the dependency problems they're dying on aren't fixed).
Maybe also do a 22-23 upgrade test and check it still succeeds
properly.

Diff Detail

Repository
rOPENQATESTS os-autoinst-distri-fedora
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
adamwill retitled this revision from to explicitly check for DNF "system is not ready for upgrade".Nov 17 2015, 7:05 PM
adamwill updated this object.
adamwill edited the test plan for this revision. (Show Details)
adamwill added reviewers: jskladan, garretraziel.
adamwill updated this revision to Diff 1693.Nov 17 2015, 7:14 PM

Missing quotes

garretraziel accepted this revision.Nov 18 2015, 1:44 PM

I've encountered this too and this patch helps a lot.

tests/upgrade_run.pm
26

I know that this is proper Perl-way but I would vote for good ol' verbose way like:

if (check_screen "upgrade_fail") {
    die "DNF reported failure";
}

sure it adds two more lines, but I consider this to be easier to skim through (because when you are reading one line after the other, you encounter "die" and think "why the hell are we want to die suddenly" and then you have to read whole line to find out that this dying is conditional and it won't happen in correct run).

If you don't agree, feel free to merge this as it is, I don't really have that much of a problem with it. Perhaps it's caused by me, trying to apply The Zen of Python on something so fundamentally different like Perl.

This revision is now accepted and ready to land.Nov 18 2015, 1:44 PM
adamwill added a comment.Nov 18 2015, 7:15 PM

I kind of hate both perl options; I'd much prefer the python 'two line, no braces' style, but I really hate the perl 'three lines, annoying braces' style - it's so finicky and eats up so much vertical space, once you have a few trivial conditionals the whole damn thing is so *long* and empty, I find that more of a readability problem than the 'effect before cause' problem with the short version. So I'm gonna merge it like this, because I hate it slightly less than the alternative.

Closed by commit rOPENQATESTS3ecef54b51e7: explicitly check for DNF "system is not ready for upgrade" (authored by adamwill). · Explain WhyNov 18 2015, 7:17 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

Diff 1695

View Options

needles/upgrade_fail.json

  • This file was added.
1​{
2​ "area": [
3​ {
4​ "height": 15,
5​ "type": "match",
6​ "width": 305,
7​ "xpos": 0,
8​ "ypos": 735
9​ }
10​ ],
11​ "tags": [
12​ "upgrade_fail",
13​ "ENV-DISTRI-fedora",
14​ "ENV-FLAVOR-server"
15​ ]
16​}
View Options

needles/upgrade_fail.png

  • This binary file was added.
View Options

tests/upgrade_run.pm

Show All 17 Lines5sub run {
18 18​
19 # wait until dnf finishes its work (screen stops moving for 30 seconds) 19​ # wait until dnf finishes its work (screen stops moving for 30 seconds)
20 wait_still_screen 30, 6000; # TODO: shorter timeout, longer stillscreen? 20​ wait_still_screen 30, 6000; # TODO: shorter timeout, longer stillscreen?
21 21​
22 upload_logs "/var/log/dnf.log"; 22​ upload_logs "/var/log/dnf.log";
23 upload_logs "/var/log/dnf.rpm.log"; 23​ upload_logs "/var/log/dnf.rpm.log";
24 24​
25 script_run "dnf system-upgrade reboot"; 25​ script_run "dnf system-upgrade reboot";
26​ die "DNF reported failure" if (check_screen "upgrade_fail");
garretrazielUnsubmitted
Not Done

I know that this is proper Perl-way but I would vote for good ol' verbose way like:

if (check_screen "upgrade_fail") {
    die "DNF reported failure";
}

sure it adds two more lines, but I consider this to be easier to skim through (because when you are reading one line after the other, you encounter "die" and think "why the hell are we want to die suddenly" and then you have to read whole line to find out that this dying is conditional and it won't happen in correct run).

If you don't agree, feel free to merge this as it is, I don't really have that much of a problem with it. Perhaps it's caused by me, trying to apply The Zen of Python on something so fundamentally different like Perl.

26 27​
27 # now offline upgrading starts. user doesn't have to do anything, just wait untill 28​ # now offline upgrading starts. user doesn't have to do anything, just wait untill
28 # system reboots and login screen is shown 29​ # system reboots and login screen is shown
29 if (get_var('UPGRADE') eq "desktop") { 30​ if (get_var('UPGRADE') eq "desktop") {
30 assert_screen "graphical_login", 6000; 31​ assert_screen "graphical_login", 6000;
31 } else { 32​ } else {
32 assert_screen "text_console_login", 6000; 33​ assert_screen "text_console_login", 6000;
33 } 34​ }
Show All 14 Lines