Dereference symlink when displaying needle info #2738

Amrysliu · 2020-02-07T15:10:20Z

See: https://progress.opensuse.org/issues/53762

Amrysliu · 2020-02-07T15:15:37Z

I set this PR as WIP because I need to add test code.
After the modification, the UI will shown as the picture in this ticket's description (after.png).

codecov · 2020-02-07T15:59:56Z

Codecov Report

Merging #2738 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2738      +/-   ##
==========================================
- Coverage   92.14%   92.14%   -0.01%     
==========================================
  Files         187      187              
  Lines       11728    11734       +6     
==========================================
+ Hits        10807    10812       +5     
- Misses        921      922       +1

Impacted Files	Coverage Δ
lib/OpenQA/WebAPI/Controller/Admin/Needle.pm	`79.31% <100%> (+2.38%)`	⬆️
lib/OpenQA/Worker/Engines/isotovideo.pm	`92.98% <0%> (-0.44%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa28b31...87486fb. Read the comment docs.

Amrysliu · 2020-02-17T10:13:43Z

I set this PR as WIP because I need to add test code.
After the modification, the UI will shown as the picture in this ticket's description (after.png).

Already added the test code. Ready for review.

Martchus

Please use e.g. $needle->path in all the places you're currently duplicating that function like $needle->directory->path . '/' . $needle->filename.

This slightly decreases the AJAX loading time (~ 0.0399 secs => ~ 0.0583 secs with OSD data) but I guess it is still ok. Good that this table is using server-side rendering.

t/ui/21-admin-needles.t

Amrysliu · 2020-02-18T14:19:34Z

Please use e.g. $needle->path in all the places you're currently duplicating that function like $needle->directory->path . '/' . $needle->filename.

This slightly decreases the AJAX loading time (~ 0.0399 secs => ~ 0.0583 secs with OSD data) but I guess it is still ok. Good that this table is using server-side rendering.

Thanks for the reminder. I did not know the path function before.

okurz · 2020-02-21T10:22:55Z

lib/OpenQA/WebAPI/Controller/Admin/Needle.pm

+            my %needles_record_real_path = map {
+                $_->path => {
+                    needle_file_real_path => realpath($_->path),
+                    last_seen             => $_->last_seen_time || 'never',
+                    last_match            => $_->last_matched_time || 'never',
+                }
+            } $needles->all;


The duplication is worrysome. I don't understand why you also need to update here what is done later in the next while-loop. We can at least extract a function for the initialisation for last_seen+last_match. Maybe you have an even better idea.

Yes. You could for instance create a simple mapping from path to DBIx object: my %needles_record_real_path = map { $_->path => $_ } $needles->all and access the DBIx object directly in the loop below where you do the "never"-fallback only one time in the end like this:

$hash->{last_seen} ||= 'never'; $hash->{last_match} ||= 'never';

I modified the code, please help to review if it is the way that you suggested. Thanks

okurz · 2020-02-21T10:24:05Z

t/ui/21-admin-needles.t

+    my $symlink_needle_dir_id = $needle_dir->create(
+        {
+            path => $symlink_needle_dir . '/needles',
+            name => 'symlink_needle_dir'
+        })->id;
+    my $real_needle_dir_id = $needle_dir->create(
+        {
+            path => $real_needle_dir . '/needles',
+            name => 'real_needle_dir'
+        })->id;


you can do a for loop to apply the same for both dirs.

I think it is ok. The assignment is to different variables and the 2 hash entries are different.

okurz · 2020-02-21T10:31:34Z

t/ui/21-admin-needles.t

+    $driver->find_element('.dataTables_filter input')->send_keys("bootloader");
+    wait_for_ajax;
+    my @needle_trs = $driver->find_elements('#needles tbody tr');


maybe you can take a look into #2626 and use wait_for_element instead of the combination of wait_for_ajax and find_elements

I tried to use wait_for_element, but failed to get the array of tr using this function.

okurz · 2020-02-21T10:32:38Z

t/ui/21-admin-needles.t

+    is((shift @symlink_needle_tds)->get_text(), 'symlink_needle_dir', 'symlink needle dir is right');
+    is((shift @symlink_needle_tds)->get_text(), 'bootloader.json',    'symlink needle file name is right');
+    is((shift @symlink_needle_tds)->get_text(), 'a day ago',          'symlink needle last use is right');
+    is((shift @symlink_needle_tds)->get_text(), 'about 14 hours ago', 'symlink needle last match is right');


I guess we can find better explanations than "… is right". Can you be more specific?

I changed is right to is displayed correct, does it make sense? :)

Martchus · 2020-02-21T10:50:18Z

lib/OpenQA/WebAPI/Controller/Admin/Needle.pm

+            my %needles_record_real_path = map {
+                $_->path => {
+                    needle_file_real_path => realpath($_->path),
+                    last_seen             => $_->last_seen_time || 'never',
+                    last_match            => $_->last_matched_time || 'never',
+                }
+            } $needles->all;


Yes. You could for instance create a simple mapping from path to DBIx object: my %needles_record_real_path = map { $_->path => $_ } $needles->all and access the DBIx object directly in the loop below where you do the "never"-fallback only one time in the end like this:

$hash->{last_seen} ||= 'never'; $hash->{last_match} ||= 'never';

lib/OpenQA/WebAPI/Controller/Admin/Needle.pm

okurz

I think it's ok like it is now

Martchus · 2020-02-24T13:25:46Z

t/ui/21-admin-needles.t

+    like(
+        $driver->find_child_element($last_matched_td, 'a')->get_attribute('href'),
+        qr/admin\/needles\/$last_matched_module_id\/$real_needle_id/,
+        'symlink needle last used module link is correctly'


Just a grammar nitpick: I believe in this case it should be 'correct' instead of 'correctly' (after is usually comes an adjective and not an adverb).

See: https://progress.opensuse.org/issues/53762

okurz · 2020-02-25T08:02:53Z

"fullstack" failed with something about "Live View". The fullstack test should retry but failed after first try. Maybe I have caused this with a recent change. I retriggered the failed test.

There is a bug in PR os-autoinst#2738. If the symlink needle is not loaded with the real needle file together, the last seen and last match of the symlink needle are still 'Never'. Related ticket: https://progress.opensuse.org/issues/53762#note-9

Amrysliu force-pushed the fix_53762 branch from a12c2ac to 2fa6f95 Compare February 17, 2020 10:10

Amrysliu changed the title ~~[WIP] Dereference symlink when displaying needle info~~ Dereference symlink when displaying needle info Feb 17, 2020

Martchus self-requested a review February 17, 2020 10:44

Martchus requested changes Feb 17, 2020

View reviewed changes

t/ui/21-admin-needles.t Outdated Show resolved Hide resolved

t/ui/21-admin-needles.t Outdated Show resolved Hide resolved

Amrysliu force-pushed the fix_53762 branch from 2fa6f95 to 55add0a Compare February 18, 2020 14:05

okurz reviewed Feb 21, 2020

View reviewed changes

Martchus reviewed Feb 21, 2020

View reviewed changes

Amrysliu force-pushed the fix_53762 branch from 55add0a to e218660 Compare February 21, 2020 15:09

okurz approved these changes Feb 22, 2020

View reviewed changes

Martchus approved these changes Feb 24, 2020

View reviewed changes

Dereference symlink when displaying needle info

87486fb

See: https://progress.opensuse.org/issues/53762

Amrysliu force-pushed the fix_53762 branch from e218660 to 87486fb Compare February 25, 2020 07:09

okurz merged commit 1a487f6 into os-autoinst:master Feb 25, 2020

Amrysliu mentioned this pull request Feb 28, 2020

Fix the bug that only dereference the symlink needle in one page #2793

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dereference symlink when displaying needle info #2738

Dereference symlink when displaying needle info #2738

Amrysliu commented Feb 7, 2020

Amrysliu commented Feb 7, 2020

codecov bot commented Feb 7, 2020 •

edited

Loading

Amrysliu commented Feb 17, 2020

Martchus left a comment

Amrysliu commented Feb 18, 2020

okurz Feb 21, 2020

Martchus Feb 21, 2020

Amrysliu Feb 21, 2020 •

edited

Loading

okurz Feb 21, 2020

Martchus Feb 21, 2020

okurz Feb 21, 2020

Amrysliu Feb 21, 2020

okurz Feb 21, 2020

Amrysliu Feb 21, 2020

Martchus Feb 21, 2020

okurz left a comment

Martchus Feb 24, 2020

okurz commented Feb 25, 2020

Dereference symlink when displaying needle info #2738

Dereference symlink when displaying needle info #2738

Conversation

Amrysliu commented Feb 7, 2020

Amrysliu commented Feb 7, 2020

codecov bot commented Feb 7, 2020 • edited Loading

Codecov Report

Amrysliu commented Feb 17, 2020

Martchus left a comment

Choose a reason for hiding this comment

Amrysliu commented Feb 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amrysliu Feb 21, 2020 • edited Loading

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

okurz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okurz commented Feb 25, 2020

codecov bot commented Feb 7, 2020 •

edited

Loading

Amrysliu Feb 21, 2020 •

edited

Loading