Simplify OpenQA::WebAPI::Controller::Admin::Needle and enhance tests #2825
Conversation
okurz
commented
Mar 11, 2020
okurz
commented
- Simplify update of seen/match in OpenQA::WebAPI::Controller::Admin::Needle
- Simplify 'prepare_data_function' as followup to Fix the bug that only dereference the symlink needle in one page #2793
- t: Bail out early on Selenium reported errors for concise log
- t: Ensure necessary test dirs exist for ui/21-admin-needles.t
okurz
force-pushed
the
enhance/needle_admin_view
branch
from
38f63bc
to
84deae9
Compare
| $dir = { | ||
| real_path => $dir_real_path, | ||
| real_path_id => $real_path_id, | ||
| }; |
There was a problem hiding this comment.
this value should be stored in $paths and used for next search.
There was a problem hiding this comment.
I think we do this correctly because $dir is a reference to an element within $paths.
There was a problem hiding this comment.
if I did a test about this correctly, the $dir is an undef in this else block, so the $dir = {} is not added into $paths
There was a problem hiding this comment.
Do you have an idea how to test this better? The automatic test is passed and also NYTProf shows the same amount of readlink calls.
There was a problem hiding this comment.
I did the test in my local test environment. I added some log message in the else block and found the $paths was always empty.
In my test, the picture1 shows that, if we use $dir->{}, the realpath will be called 25 times. If we add the value to $paths->{$dir_path} = {} directly, the realpath will be called 1 time. BTW, the needle page loads 25 needles at one time and all of them are belong to one needle directory which is a symlink.
There was a problem hiding this comment.
I managed to come up to this story. Thanks for sharing your information. This helps as reference.
I gathered coverage from runs of t/ui/21-admin-needles.t and found 5 vs. 18 calls of reallink in case of my "broken" code and the fixed code.
I pushed the fix.
okurz
force-pushed
the
enhance/needle_admin_view
branch
from
84deae9
to
6b52365
Compare
| return \@data; | ||
| my $needle_dirs = $self->schema->resultset('NeedleDirs'); | ||
| my $needles = $self->schema->resultset('Needles'); | ||
| my %paths = {}; |
There was a problem hiding this comment.
maybe this hash should be written my %paths = ();
There was a problem hiding this comment.
Actually, just write it as my %paths; without any assignment. There's no such thing as an uninitialized hash in Perl; you always get an empty/default-initialized one.
When running tests on a pristine working copy the '$needle_dir' did not exist failing the test. This commit ensures the directory tree to exist and also simplifies the content writing.
When the Selenium error handler catches errors it can be repeated a
thousand time polluting the console scrollback buffer so that the root
cause problem can not be found. This commit changes the call from "fail"
to "BAIL_OUT" to abort the test run early.
So instead of this:
```
not ok 1 - executeScript: unexpected alert open: {Alert text : DataTables warning: table id=needles - Ajax error. For more information about this error, please see http://datatables.net/tn/7}
…
not ok 998 - executeScript: unexpected alert open: {Alert text : DataTables warning: table id=needles - Ajax error. For more information about this error, please see http://datatables.net/tn/7}
not ok 999 - executeScript: unexpected alert open: {Alert text : DataTables warning: table id=needles - Ajax error. For more information about this error, please see http://datatables.net/tn/7}
```
have this:
```
Bailout called. Further testing stopped: executeScript: unexpected alert open: {Alert text : DataTables warning: table id=needles - Ajax error. For more information about this error, please see http://datatables.net/tn/7}
Bail out! executeScript: unexpected alert open: {Alert text : DataTables warning: table id=needles - Ajax error. For more information about this error, please see http://datatables.net/tn/7}
FAILED--Further testing stopped: executeScript: unexpected alert open: {Alert text : DataTables warning: table id=needles - Ajax error. For more information about this error, please see http://datatables.net/tn/7} at ../lib/OpenQA/SeleniumTest.pm line 107.
```
By pulling out a function we can reduce the length of the "ajax" method and replace the explicit while loop with a map call which can even be more efficient.
okurz
force-pushed
the
enhance/needle_admin_view
branch
2 times, most recently
from
e95de0c
to
e6651b9
Compare
