From c4edf8009efeaf285aefe4799e6f41aa6d511326 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Mon, 31 Oct 2016 11:41:10 -0700
Subject: [PATCH] Improve and simplify post_fail_hook existence checks

Summary:
The current installedtest post_fail_hook assumes /var/tmp/abrt
exists at all, and dies if it doesn't, leading to no /var/log
upload. We can also avoid using openQA `script_output` - which
is annoyingly indirect and slow - by using this neat `test -n`
trick I found on SO. Let's also use it in the anacondatest
post_fail_hook to avoid uploading /var/tmp when it's empty
(which we currently do). This also drops the 0 arg from a few
more script_run calls, because it's safe to wait for the run
to complete and we should probably do so to avoid later typing
errors if the commands are slow.

Test Plan:
Cause both anaconda and installed tests to fail and
check the hooks work as intended. Maybe twiddle the failures to
ensure directories do and don't exist and/or have contents and
make sure things work OK. I've tested this to some degree and
I'm pretty sure it works right.

Reviewers: jskladan, garretraziel

Reviewed By: garretraziel

Subscribers: tflink

Differential Revision: https://phab.qadevel.cloud.fedoraproject.org/D1041
---
 lib/anacondatest.pm  | 19 +++++++++++--------
 lib/installedtest.pm | 19 ++++++++-----------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/lib/anacondatest.pm b/lib/anacondatest.pm
index ae5ea9b9..ef02d10d 100644
--- a/lib/anacondatest.pm
+++ b/lib/anacondatest.pm
@@ -36,21 +36,24 @@ sub post_fail_hook {
 
     if ($has_traceback) {
         # Upload Anaconda traceback logs
-        script_run "tar czf /tmp/anaconda_tb.tar.gz /tmp/anaconda-tb-*", 0;
+        script_run "tar czf /tmp/anaconda_tb.tar.gz /tmp/anaconda-tb-*";
         upload_logs "/tmp/anaconda_tb.tar.gz";
     }
 
-    # Upload all ABRT logs
-    script_run "tar czf /var/tmp/var_tmp.tar.gz /var/tmp", 0;
-    upload_logs "/var/tmp/var_tmp.tar.gz";
+    # Upload all ABRT logs (if there are any)
+    unless (script_run 'test -n "$(ls -A /var/tmp)" && tar czf /var/tmp/var_tmp.tar.gz /var/tmp') {
+        upload_logs "/var/tmp/var_tmp.tar.gz";
+    }
 
     # Upload /var/log
-    script_run "tar czf /tmp/var_log.tar.gz /var/log", 0;
-    upload_logs "/tmp/var_log.tar.gz";
+    unless (script_run "tar czf /tmp/var_log.tar.gz /var/log") {
+        upload_logs "/tmp/var_log.tar.gz";
+    }
 
     # Upload anaconda core dump, if there is one
-    script_run "ls /tmp/anaconda.core.* && tar czf /tmp/anaconda.core.tar.gz /tmp/anaconda.core.*", 0;
-    upload_logs "/tmp/anaconda.core.tar.gz", failok=>1;
+    unless (script_run "ls /tmp/anaconda.core.* && tar czf /tmp/anaconda.core.tar.gz /tmp/anaconda.core.*") {
+        upload_logs "/tmp/anaconda.core.tar.gz";
+    }
 }
 
 sub root_console {
diff --git a/lib/installedtest.pm b/lib/installedtest.pm
index 852389e0..2a17dd1c 100644
--- a/lib/installedtest.pm
+++ b/lib/installedtest.pm
@@ -27,24 +27,21 @@ sub post_fail_hook {
     # We can't rely on tar being in minimal installs
     assert_script_run "dnf -y install tar", 180;
 
-    # If /var/tmp/abrt directory isn't empty (ls doesn't return empty string)
-    my $vartmp = script_output "ls /var/tmp/abrt";
-    if ($vartmp ne '') {
-        # Upload /var/tmp ABRT logs
-        script_run "cd /var/tmp/abrt && tar czvf tmpabrt.tar.gz *", 0;
+    # Note: script_run returns the exit code, so the logic looks weird.
+    # We're testing that the directory exists and contains something.
+    unless (script_run 'test -n "$(ls -A /var/tmp/abrt)" && cd /var/tmp/abrt && tar czvf tmpabrt.tar.gz *') {
         upload_logs "/var/tmp/abrt/tmpabrt.tar.gz";
     }
-    my $varspool = script_output "ls /var/spool/abrt";
-    if ($varspool ne '') {
-        # Upload /var/spool ABRT logs
-        script_run "cd /var/spool/abrt && tar czvf spoolabrt.tar.gz *", 0;
+
+    unless (script_run 'test -n "$(ls -A /var/spool/abrt)" && cd /var/spool/abrt && tar czvf spoolabrt.tar.gz *') {
         upload_logs "/var/spool/abrt/spoolabrt.tar.gz";
     }
 
     # Upload /var/log
     # lastlog can mess up tar sometimes and it's not much use
-    script_run "tar czvf /tmp/var_log.tar.gz --exclude='lastlog' /var/log", 0;
-    upload_logs "/tmp/var_log.tar.gz";
+    unless (script_run "tar czvf /tmp/var_log.tar.gz --exclude='lastlog' /var/log") {
+        upload_logs "/tmp/var_log.tar.gz";
+    }
 }
 
 sub check_release {