From 05ae233a6a75fdab9b8a1bfde09c8a40a8c190b8 Mon Sep 17 00:00:00 2001 From: Oliver Kurz Date: Fri, 13 Dec 2019 10:26:40 +0100 Subject: [PATCH] Fix undefined needles dir in dynamic needle downloader with proper variable names It seems a subtle, unintended difference in variable names caused problems within the openQA developer mode when one tries to reload the needles that had just been created with the needle editor during the same run. There was a warning ``` Use of uninitialized value $needle_dir in concatenation (.) or string at /usr/lib/os-autoinst/OpenQA/Isotovideo/NeedleDownloader.pm line 76. ``` which did not cause any more severe effects so the error could not be seen earlier unfortunately. I chose the name "needles_dir" also corresponding to the external test variable "NEEDLES_DIR". Related progress issue: https://progress.opensuse.org/issues/61019 --- OpenQA/Isotovideo/NeedleDownloader.pm | 4 ++-- needle.pm | 28 +++++++++++++-------------- t/01-test_needle.t | 10 +++++----- t/02-test_ocr.t | 2 +- t/17-basetest.t | 4 ++-- t/21-needle-downloader.t | 18 ++++++++--------- 6 files changed, 33 insertions(+), 33 deletions(-) diff --git a/OpenQA/Isotovideo/NeedleDownloader.pm b/OpenQA/Isotovideo/NeedleDownloader.pm index 62aa41e630b..34f84ca07a1 100644 --- a/OpenQA/Isotovideo/NeedleDownloader.pm +++ b/OpenQA/Isotovideo/NeedleDownloader.pm @@ -72,8 +72,8 @@ sub _add_download { my $needle_name = $needle->{name}; my $latest_update = $needle->{t_updated}; - my $needle_dir = $needle::needles_dir; - my $download_target = "$needle_dir/$needle_name.$extension"; + my $needles_dir = $needle::needles_dir; + my $download_target = "$needles_dir/$needle_name.$extension"; if (my $target_stat = stat($download_target)) { if (my $target_last_modified = $target_stat->[9] // $target_stat->[8]) { diff --git a/needle.pm b/needle.pm index 78d26e773a7..1e9c8c12bca 100644 --- a/needle.pm +++ b/needle.pm @@ -33,7 +33,7 @@ use OpenQA::Benchmark::Stopwatch; our %needles; our %tags; -our $needledir; +our $needles_dir; our $cleanuphandler; sub is_click_point_valid { @@ -47,12 +47,12 @@ sub is_click_point_valid { sub new { my ($classname, $jsonfile) = @_; - die 'needles not initialized via needle::init() before needle constructor called' unless defined $needledir; + die 'needles not initialized via needle::init() before needle constructor called' unless defined $needles_dir; my $json; if (ref $jsonfile eq 'HASH') { $json = $jsonfile; - $jsonfile = $json->{file} || File::Spec->catfile($needledir, $json->{name} . '.json'); + $jsonfile = $json->{file} || File::Spec->catfile($needles_dir, $json->{name} . '.json'); } my $self = {}; @@ -61,16 +61,16 @@ sub new { # - This code initializes $json->{file} so it contains the path within the needle directory. # - $jsonfile is re-assigned to contain the absolute path the the JSON file. # - The needle must be within the needle directory. - if (index($jsonfile, $needledir) == 0) { - $self->{file} = substr($jsonfile, length($needledir) + 1); + if (index($jsonfile, $needles_dir) == 0) { + $self->{file} = substr($jsonfile, length($needles_dir) + 1); } - elsif (-f File::Spec->catfile($needledir, $jsonfile)) { + elsif (-f File::Spec->catfile($needles_dir, $jsonfile)) { # json file path already relative $self->{file} = $jsonfile; - $jsonfile = File::Spec->catfile($needledir, $jsonfile); + $jsonfile = File::Spec->catfile($needles_dir, $jsonfile); } else { - die "Needle $jsonfile is not under needle directory $needledir"; + die "Needle $jsonfile is not under needle directory $needles_dir"; } if (!$json) { @@ -323,21 +323,21 @@ sub default_needles_dir { } sub init { - $needledir = ($bmwqemu::vars{NEEDLES_DIR} // default_needles_dir); - $needledir = File::Spec->catdir($bmwqemu::vars{CASEDIR}, $needledir) unless -d $needledir; - die "needledir not found: $needledir (check vars.json?)" unless -d $needledir; + $needles_dir = ($bmwqemu::vars{NEEDLES_DIR} // default_needles_dir); + $needles_dir = File::Spec->catdir($bmwqemu::vars{CASEDIR}, $needles_dir) unless -d $needles_dir; + die "needles_dir not found: $needles_dir (check vars.json?)" unless -d $needles_dir; %needles = (); %tags = (); - bmwqemu::diag("init needles from $needledir"); - find({no_chdir => 1, wanted => \&wanted_, follow => 1}, $needledir); + bmwqemu::diag("init needles from $needles_dir"); + find({no_chdir => 1, wanted => \&wanted_, follow => 1}, $needles_dir); bmwqemu::diag(sprintf("loaded %d needles", scalar keys %needles)); if ($cleanuphandler) { &$cleanuphandler(); } - return $needledir; + return $needles_dir; } sub tags { diff --git a/t/01-test_needle.t b/t/01-test_needle.t index 095bc8ca7ee..e1e6e55e3cb 100755 --- a/t/01-test_needle.t +++ b/t/01-test_needle.t @@ -36,8 +36,8 @@ require tinycv; my ($res, $needle, $img1, $cand); -my $data_dir = dirname(__FILE__) . '/data/'; -my $misc_needle_dir = abs_path(dirname(__FILE__)) . '/misc_needles/'; +my $data_dir = dirname(__FILE__) . '/data/'; +my $misc_needles_dir = abs_path(dirname(__FILE__)) . '/misc_needles/'; $bmwqemu::vars{NEEDLES_DIR} = $data_dir; needle::init; @@ -381,7 +381,7 @@ subtest 'needle::init accepts custom NEEDLES_DIR within working directory and ot my $needles_dir = $bmwqemu::vars{NEEDLES_DIR} = "$temp_working_dir/some-needle-repo"; make_path("$needles_dir/subdir"); for my $extension (qw(json png)) { - Mojo::File->new($misc_needle_dir, "click-point.$extension")->copy_to("$needles_dir/subdir/foo.$extension"); + Mojo::File->new($misc_needles_dir, "click-point.$extension")->copy_to("$needles_dir/subdir/foo.$extension"); } subtest 'custom NEEDLES_DIR used when within working directory' => sub { @@ -397,7 +397,7 @@ subtest 'needle::init accepts custom NEEDLES_DIR within working directory and ot }; subtest 'click point' => sub { - $needle::needledir = $misc_needle_dir; + $needle::needles_dir = $misc_needles_dir; my $needle = needle->new('click-point.json'); is_deeply($needle->{area}->[0]->{click_point}, {xpos => 2, ypos => 4}, 'click point parsed'); @@ -412,7 +412,7 @@ subtest 'click point' => sub { }; subtest 'workaround property' => sub { - $needle::needledir = $misc_needle_dir; + $needle::needles_dir = $misc_needles_dir; my $workaround_string_needle = needle->new('check-workaround-bsc1234567-20190522.json'); my $workaround_hash_needle = needle->new('check-workaround-hash-20190522.json'); diff --git a/t/02-test_ocr.t b/t/02-test_ocr.t index d1ee80ac2c6..cd70e908dd9 100755 --- a/t/02-test_ocr.t +++ b/t/02-test_ocr.t @@ -28,7 +28,7 @@ unless (which('tesseract')) { needle::init; -my $img1 = tinycv::read("$needle::needledir/bootmenu.test.png"); +my $img1 = tinycv::read("$needle::needles_dir/bootmenu.test.png"); my $needle = needle->new('bootmenu-ocr.ref.json'); my $res = $img1->search($needle); ok(defined $res, 'ocr match 1'); diff --git a/t/17-basetest.t b/t/17-basetest.t index 37719cd74ae..a8a2d5456ba 100755 --- a/t/17-basetest.t +++ b/t/17-basetest.t @@ -220,8 +220,8 @@ subtest record_screenmatch => sub { # check a needle has workaround property my $basetest_for_workaround = basetest->new(); - my $misc_needle_dir = dirname(__FILE__) . '/misc_needles/'; - my $needle_file = $misc_needle_dir . "check-workaround-hash-20190522.json"; + my $misc_needles_dir = dirname(__FILE__) . '/misc_needles/'; + my $needle_file = $misc_needles_dir . "check-workaround-hash-20190522.json"; my %workaround_match = ( area => [ {x => 1, y => 2, w => 3, h => 4, result => 'ok'}, diff --git a/t/21-needle-downloader.t b/t/21-needle-downloader.t index a5966de64bd..3da1e6ba1e4 100755 --- a/t/21-needle-downloader.t +++ b/t/21-needle-downloader.t @@ -23,8 +23,8 @@ $user_agent_mock->mock(get => sub { }); # setup needle directory -my $needle_dir = $needle::needles_dir = path(tempdir, 'needle_dir'); -ok(make_path($needle_dir), 'create test needle dir under ' . $needle_dir); +my $needles_dir = $needle::needles_dir = path(tempdir, 'needles_dir'); +ok(make_path($needles_dir), 'create test needle dir under ' . $needles_dir); subtest 'deduce URL for needle download from test variable OPENQA_URL' => sub { $bmwqemu::vars{OPENQA_URL} = 'https://openqa1-opensuse'; @@ -70,22 +70,22 @@ subtest 'add relevant downloads' => sub { # pretend that ... # ... one file is already up to date (to the exact second) - File::Touch->new(mtime => 1514764800)->touch($needle_dir . '/foo.png'); + File::Touch->new(mtime => 1514764800)->touch($needles_dir . '/foo.png'); # ... one file is present but outdated (by one second) - File::Touch->new(mtime => 1514764799)->touch($needle_dir . '/bar.json'); + File::Touch->new(mtime => 1514764799)->touch($needles_dir . '/bar.json'); # define expected downloads: everything from @new_needles except foo.png my @expected_downloads = ( { - target => $needle_dir . '/foo.json', + target => $needles_dir . '/foo.json', url => 'http://openqa/needles/1/json', }, { - target => $needle_dir . '/bar.json', + target => $needles_dir . '/bar.json', url => 'http://openqa/needles/2/json', }, { - target => $needle_dir . '/bar.png', + target => $needles_dir . '/bar.png', url => 'http://openqa/needles/2/image', } ); @@ -95,7 +95,7 @@ subtest 'add relevant downloads' => sub { sub { $downloader->add_relevant_downloads(\@new_needles); }, - qr/.*skipping downloading new needle: $needle_dir\/foo\.png seems already up-to-date.*/, + qr/.*skipping downloading new needle: $needles_dir\/foo\.png seems already up-to-date.*/, 'skipped downloads logged' ); is_deeply($downloader->files_to_download, \@expected_downloads, 'downloads added') @@ -127,6 +127,6 @@ subtest 'download added URLs' => sub { ], 'right URLs queried'); }; -remove_tree($needle_dir); +remove_tree($needles_dir); done_testing;