Add `mouse_drag` subroutine to support click and drag in tests #1508

lruzicka · 2020-08-17T11:47:41Z

This PR adds the mouse_drag subroutine that enables the mouse and drag
support for OpenQA tests. To use the subroutine, it is possible to either
provide a "starting" and "ending" needle from which the starting and finishing
points will be calculated (using the centres of the needle tag), or
coordinates might be provided directly. If both needles and coordinates are
provided at the same time, then the coordinates precede over the needles.
Combinations are possible, too, which means that one of the points can
be passed as a needle and the other as coordinates.
In order not to repete code, the part responsible for calculating the
center of needle was removed from the click_lastmatch subroutine and moved
into a standalone non-exported subroutine called calculate_clickpoint to be
also used by the mouse_drag subroutine, as well as by the original
click_lastmatch.

Martchus

If there's a related ticket, please mention it in the commit message. It would also be appreciated to extend the unit test the new code (e.g. t/03-testapi.t). Besides the mentioned points the PR looks good.

testapi.pm

Martchus · 2020-08-17T15:31:44Z

Looks like all issues I've mentioned have been addressed but tests are still missing. Not that the failing ./18-qemu.t in the OBS build is unrelated. (It fails on my own PR as well and I'll have to look into it.)

Seems like the behavior is still messed up when there's a needle with the click point explicitly set to center:

6: Can't use string ("center") as a HASH ref while "strict refs" in use at /opt/testapi.pm line 562.
6: # Tests were run but no plan was declared and done_testing() was not seen.
6: # Looks like your test exited with 255 just after 25.
6: ./03-testapi.t ...........................

testapi.pm

codecov · 2020-08-24T10:21:34Z

Codecov Report

Merging #1508 into master will increase coverage by 1.42%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1508      +/-   ##
==========================================
+ Coverage   56.37%   57.79%   +1.42%     
==========================================
  Files          54       55       +1     
  Lines        6386     6499     +113     
==========================================
+ Hits         3600     3756     +156     
+ Misses       2786     2743      -43

Impacted Files	Coverage Δ
opt/OpenQA/Isotovideo/Utils.pm	`82.14% <0.00%> (-5.70%)`	⬇️
opt/backend/generalhw.pm	`81.48% <0.00%> (ø)`
opt/isotovideo	`87.04% <0.00%> (+0.15%)`	⬆️
opt/commands.pm	`14.43% <0.00%> (+0.44%)`	⬆️
opt/consoles/VNC.pm	`5.22% <0.00%> (+1.30%)`	⬆️
opt/testapi.pm	`60.51% <0.00%> (+1.40%)`	⬆️
opt/backend/baseclass.pm	`46.19% <0.00%> (+2.44%)`	⬆️
opt/consoles/vnc_base.pm	`30.30% <0.00%> (+20.20%)`	⬆️
opt/consoles/console.pm	`78.26% <0.00%> (+23.91%)`	⬆️
opt/consoles/network_console.pm	`83.33% <0.00%> (+41.66%)`	⬆️

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 45b1092...168d443. Read the comment docs.

Martchus

The code looks good now beside one style issue. I've restarted the CI because it failed to upload coverage data. But even without the report is is clear that the PR would decrease test coverage because not tests for the new test API function are provided. Having a test for the mouse_drag function would be desirable.

testapi.pm

1. This PR adds the `mouse_drag` subroutine that enables the mouse and drag support for OpenQA tests. To use the subroutine, it is possible to either provide a "starting" and "ending" needle from which the starting and finishing points will be calculated (using the centres of the needle tag), or coordinates might be provided directly. If both needles and coordinates are provided at the same time, then the coordinates precede over the needles. Combinations are possible, too, which means that one of the points can be passed as a needle and the other as coordinates. 2. In order not to repete code, the part responsible for calculating the center of needle was removed from the `click_lastmatch` subroutine and moved into a standalone non-exported subroutine called `calculate_clickpoint` to be also used by the `mouse_drag` subroutine, as well as by the original `click_lastmatch`. 3. This PR solves action #39335

lruzicka · 2020-08-24T15:52:38Z

I am happy that the PR now passes the required test. I will try to provide the tests for the mouse_drag function as you request and also for the _calculate_clickpoint. Just let me figure it out. 😺

t/03-testapi.t

Martchus · 2020-08-25T11:14:34Z

t/03-testapi.t

+
+    $cmds = [];
+    # Everything is provided.
+    my ($x, $y) = testapi::_calculate_clickpoint('foo', %fake_needle_area, %fake_click_point));


Looks like there's one ) too much. Besides, you should likely pass the hashes as reference, e.g. … \%fake_needle_area, \%fake_click_point ….

What is the difference between this suggestion and the original one? What does "passing as reference" means? Sorry, told you I am not quite experienced in Perl :D

in perl, when e.g. calling a subroutine with an arg that's an array or hash, you can either send the actual array or hash itself, or you can send a reference to it. A reference is a scalar which refers to a hash or an array (or other things - you can have a coderef, for instance, a reference to a block of code).

You usually don't have to worry about this in Python because Python takes care of it for you and doesn't give you any choices.

https://perldoc.perl.org/perlreftut.html is a good starting point on this stuff. The example @Martchus gave is the way to create a reference to an existing hash: if you have a hash %foo, then \%foo creates a reference to that hash. You can pass an anonymous reference when calling a subroutine - e.g. mysub(\%foo); calls mysub with an anonymous reference to %foo as the arg - or you can assign it to a scalar: my $fooref = \%foo; and then do stuff with the scalar, e.g. mysub($fooref);.

If you want a subroutine to take arrays or hashes as args it's almost always best to use references, because passing actual arrays and hashes as subroutine args doesn't really work the way you might expect. Especially if the sub takes more than one arg.

if you look at _calculate_checkpoint itself, you can see it's expecting to be passed scalars:

sub _calculate_clickpoint { my ($needle_to_use, $needle_area, $click_point) = @_;

and the rest of the code actually shows that $needle_area and $click_point are (mostly) expected to be hash refs, once you understand references.

if (!$click_point || $click_point eq 'center') { $click_point = { xpos => $needle_area->{w} / 2, ypos => $needle_area->{h} / 2, }; }

First off, see that use of ->? That is the convenient way to get a value out of a hash reference. It's not how you'd get the value out of a hash. If you had a hash %needle_area you'd use $needle_area{w}.

You can also see that here we're handling the case where $click_point is not passed at all or is passed not as a hash ref but as the special string value 'center', and what we're actually doing in those cases is redefining $click_point as a hash ref. Note we use curly braces to define $click_point here: that is a way to create a hash ref directly, as opposed to creating a named hash then creating a reference to it. If you were just creating a hash you'd do %click_point = (<stuff>) - you use regular braces. Doing $click_point = {<stuff>}, with curly braces, creates a new hash ref.

Thank you very much, Adam, for this explanation.

Martchus · 2020-08-25T11:15:12Z

t/03-testapi.t

+    ok($x == 120, 'xpoint correct');
+    ok($y == 110, 'ypoint correct');


Please use is here (for better output when the check fails).

AdamWill · 2020-08-26T19:02:19Z

I think the problem you're having with the test failures is because you're doing ok(mouse_drag(blahblah));

Per the docs, ok's behaviour is: evaluate whatever expression it's given, and pass if the result is truth-y. Fail if the result is not truth-y.

Why's it failing? Because mouse_drag doesn't explicitly return anything, which means it actually returns undef. Which is false-y. There's nothing wrong with a sub that returns undef, but it does mean you can't test it with ok. Instead you probably should just run it, and then check the results were what you expected - i.e. drop the use of ok entirely and just rely on the check of $cmds afterwards to make sure the mouse_drag call did what it was supposed to.

If you look, this is what all the other tests in the file do. The send_key tests, for e.g., don't do ok(send_key 'ret');, they just do send_key 'ret'; and then check $cmds.

Martchus · 2020-08-27T08:16:01Z

Because mouse_drag doesn't explicitly return anything, which means it actually returns undef.

Technically it returns the return value of the last statement but that appears to be undef and is nothing we want to check here.

AdamWill · 2020-08-27T18:37:28Z

right, sorry, I elided that a bit :)

Martchus · 2020-08-28T10:32:45Z

t/03-testapi.t

+                button => 'left',
+                cmd    => 'backend_mouse_button'
+            },
+    ], 'mouse drag (using coordinates only) was succesful') or diag explain $cmds;


I guess successful is written differently but I'd actually remove it completely because that's kind of implied for a test description. The same counts for the other occurrences.

lruzicka · 2020-08-28T11:26:32Z

This might be ready to merge. Thanks for all your support.

lruzicka force-pushed the master branch 4 times, most recently from 8de037f to f96596f Compare August 17, 2020 13:39

Martchus requested changes Aug 17, 2020

View reviewed changes

lruzicka force-pushed the master branch 4 times, most recently from 438c124 to 57a0593 Compare August 17, 2020 14:59

kalikiana reviewed Aug 20, 2020

View reviewed changes

testapi.pm Outdated Show resolved Hide resolved

lruzicka force-pushed the master branch 9 times, most recently from dfae2db to 4cbb3d8 Compare August 24, 2020 10:09

Martchus reviewed Aug 24, 2020

View reviewed changes

testapi.pm Outdated Show resolved Hide resolved

lruzicka force-pushed the master branch from 75aae2b to 7bd6944 Compare August 24, 2020 15:05

lruzicka force-pushed the master branch from 7bd6944 to 6442f88 Compare August 24, 2020 15:40

lruzicka force-pushed the master branch 2 times, most recently from 5ddacbb to a77e482 Compare August 25, 2020 10:51

Martchus requested changes Aug 25, 2020

View reviewed changes

lruzicka force-pushed the master branch from a77e482 to dbbc045 Compare August 25, 2020 11:25

lruzicka force-pushed the master branch 2 times, most recently from f37b800 to 7556fe0 Compare August 25, 2020 14:12

Write tests for _calculate_clickpoint

5e6d7d1

lruzicka force-pushed the master branch 12 times, most recently from 5dc9a92 to 20a834e Compare August 26, 2020 13:38

lruzicka force-pushed the master branch 5 times, most recently from 934b01c to 725c23c Compare August 28, 2020 10:18

Martchus reviewed Aug 28, 2020

View reviewed changes

lruzicka force-pushed the master branch from 725c23c to 509ac31 Compare August 28, 2020 10:34

Write tests for mouse_drag

168d443

lruzicka force-pushed the master branch from 509ac31 to 168d443 Compare August 28, 2020 11:05

Martchus approved these changes Aug 28, 2020

View reviewed changes

mergify bot merged commit e6371b1 into os-autoinst:master Aug 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `mouse_drag` subroutine to support click and drag in tests #1508

Add `mouse_drag` subroutine to support click and drag in tests #1508

lruzicka commented Aug 17, 2020

Martchus left a comment

Martchus commented Aug 17, 2020 •

edited

codecov bot commented Aug 24, 2020 •

edited

Martchus left a comment

lruzicka commented Aug 24, 2020

Martchus Aug 25, 2020

lruzicka Aug 25, 2020

AdamWill Aug 25, 2020 •

edited

AdamWill Aug 25, 2020

lruzicka Aug 26, 2020

Martchus Aug 25, 2020

AdamWill commented Aug 26, 2020 •

edited

Martchus commented Aug 27, 2020

AdamWill commented Aug 27, 2020

Martchus Aug 28, 2020 •

edited

lruzicka Aug 28, 2020

lruzicka commented Aug 28, 2020

		ok($x == 120, 'xpoint correct');
		ok($y == 110, 'ypoint correct');

Add mouse_drag subroutine to support click and drag in tests #1508

Add mouse_drag subroutine to support click and drag in tests #1508

Conversation

lruzicka commented Aug 17, 2020

Martchus left a comment

Choose a reason for hiding this comment

Martchus commented Aug 17, 2020 • edited

codecov bot commented Aug 24, 2020 • edited

Codecov Report

Martchus left a comment

Choose a reason for hiding this comment

lruzicka commented Aug 24, 2020

Martchus Aug 25, 2020

Choose a reason for hiding this comment

lruzicka Aug 25, 2020

Choose a reason for hiding this comment

AdamWill Aug 25, 2020 • edited

Choose a reason for hiding this comment

AdamWill Aug 25, 2020

Choose a reason for hiding this comment

lruzicka Aug 26, 2020

Choose a reason for hiding this comment

Martchus Aug 25, 2020

Choose a reason for hiding this comment

AdamWill commented Aug 26, 2020 • edited

Martchus commented Aug 27, 2020

AdamWill commented Aug 27, 2020

Martchus Aug 28, 2020 • edited

Choose a reason for hiding this comment

lruzicka Aug 28, 2020

Choose a reason for hiding this comment

lruzicka commented Aug 28, 2020

Add `mouse_drag` subroutine to support click and drag in tests #1508

Add `mouse_drag` subroutine to support click and drag in tests #1508

Martchus commented Aug 17, 2020 •

edited

codecov bot commented Aug 24, 2020 •

edited

AdamWill Aug 25, 2020 •

edited

AdamWill commented Aug 26, 2020 •

edited

Martchus Aug 28, 2020 •

edited