From 6e9213f4b79831c72501d75f615d1ea77801b603 Mon Sep 17 00:00:00 2001 From: Adam Williamson Date: Fri, 9 Dec 2022 15:02:43 -0800 Subject: [PATCH] Drop use of _ADVISORY_REPO_DONE, always use file tests This is safer if the advisory stuff was done on a previous test run. Hilariously, this exposed a dumb mistake I made years ago in installedtest.pm and never noticed before: the calls to advisory_* at the bottom of that file are meant to be in the post_fail_hook, but they weren't, which meant they got called by the scheduler. This didn't cause any failures because the first line caused them to return immediately based on a get_var call (which it's OK to do in the scheduler), but changing it to a script_run call (which it's *not* OK to do in scheduling) caused all the tests to blow up immediately and confused me *a lot* until I spotted this! Signed-off-by: Adam Williamson --- lib/installedtest.pm | 11 ++++++----- lib/utils.pm | 21 +++++++++++---------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/lib/installedtest.pm b/lib/installedtest.pm index da0ca7cf..1702bd4b 100644 --- a/lib/installedtest.pm +++ b/lib/installedtest.pm @@ -158,12 +158,13 @@ sub post_fail_hook { upload_logs "/var/tmp/imgbuild/lorax.log", failok => 1; upload_logs "/var/tmp/imgbuild/program.log", failok => 1; } -} -# For update tests, let's do the update package info log stuff, -# it may be useful for diagnosing the cause of the failure -advisory_get_installed_packages; -advisory_check_nonmatching_packages(fatal => 0); + # For update tests, let's do the update package info log stuff, + # it may be useful for diagnosing the cause of the failure + advisory_get_installed_packages; + advisory_check_nonmatching_packages(fatal => 0); + +} 1; diff --git a/lib/utils.pm b/lib/utils.pm index 97c2c681..7c4fd99d 100644 --- a/lib/utils.pm +++ b/lib/utils.pm @@ -553,7 +553,7 @@ sub _repo_setup_compose { sub _repo_setup_updates { # Appropriate repo setup steps for testing a Bodhi update # Check if we already ran, bail if so - return if (get_var("_ADVISORY_REPO_DONE")); + return unless script_run "test -f /mnt/updatepkgs.txt"; my $version = get_var("VERSION"); my $currrel = get_var("CURRREL", "0"); my $arch = get_var("ARCH"); @@ -677,9 +677,6 @@ sub _repo_setup_updates { type_string "exit\n"; wait_still_screen 5; } - # mark via a variable that we've set up the update/task repo and done - # all the logging stuff above - set_var('_ADVISORY_REPO_DONE', '1'); } sub repo_setup { @@ -1106,12 +1103,14 @@ sub quit_with_shortcut { } +# For update tests (this only works if we've been through +# _repo_setup_updates), figure out which packages from the update +# are currently installed. This is here so we can do it both in +# _advisory_post and post_fail_hook. sub advisory_get_installed_packages { - # For update tests (this only works if we've been through - # _repo_setup_updates), figure out which packages from the update - # are currently installed. This is here so we can do it both in - # _advisory_post and post_fail_hook. - return unless (get_var("_ADVISORY_REPO_DONE")); + # bail out if the file doesn't exist: this is in case we get + # here in the post-fail hook but we failed before creating it + return if script_run "test -f /mnt/updatepkgs.txt"; assert_script_run 'rpm -qa --qf "%{SOURCERPM} %{EPOCH} %{NAME}-%{VERSION}-%{RELEASE}\n" | sort -u > /tmp/allpkgs.txt', timeout => 90; # this finds lines which appear in both files # http://www.unix.com/unix-for-dummies-questions-and-answers/34549-find-matching-lines-between-2-files.html @@ -1137,7 +1136,9 @@ sub advisory_check_nonmatching_packages { fatal => 1, @_ ); - return unless (get_var("_ADVISORY_REPO_DONE")); + # bail out if the file doesn't exist: this is in case we get + # here in the post-fail hook but we failed before creating it + return if script_run "test -f /mnt/updatepkgnames.txt"; # if this fails in advisory_post, we don't want to do it *again* # unnecessarily in post_fail_hook return if (get_var("_ACNMP_DONE"));