Upgrade JS syn to v0.15.0 #358

mvorisek · 2022-10-16T17:12:38Z

syn.js downloaded by npm install syn and moved from node_modules/syn/dist/global/syn.js (no local compilation, only non-minified version is available, thankfully, as it is much better for debugging)

replaces #333

fixes #279 and maybe also #20

codecov · 2022-10-16T22:14:13Z

Codecov Report

Merging #358 (0405fe7) into master (5d15043) will increase coverage by 0.60%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #358      +/-   ##
============================================
+ Coverage     90.02%   90.62%   +0.60%     
  Complexity      150      150              
============================================
  Files             1        1              
  Lines           421      448      +27     
============================================
+ Hits            379      406      +27     
  Misses           42       42

Impacted Files	Coverage Δ
src/Selenium2Driver.php	`90.62% <100.00%> (+0.60%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/Selenium2Driver.php

stof · 2022-10-17T12:45:36Z

src/Selenium2Driver.php

@@ -222,25 +222,29 @@ protected function withSyn()
    /**
     * Creates some options for key events
     *
-     * @param string $char     the character or code
-     * @param string $modifier one of 'shift', 'alt', 'ctrl' or 'meta'
+     * @param string|int  $char     the character or code


why adding support for integers there ? This looks like a new feature

see #358 (comment), the method is called from Selenium2Driver::keyXxx() methods, so this is a phpdoc fix to match 1:1 the possible input types

This method is protected and not tagged as @internal. So someone extending the class (and I know some people do extend this class) could rely on it.

Well, it is not only a phpdoc fix. You also change the code to implement it

#358 (comment) resolved, the phpdoc is changed correctly and int can/is passed by the testing suite

@stof can this PR be merged or is there anything else to address?

As the upgraded syn.js lib might cause edge case incompatibilities please tag with increased minor version, ie. 1.7.0.

if integers are passed without being handled explicitly here, how did it work before ? Were we dispatching broken events (in which case it mean that this is not actually tested by the driver testsuite as it was passing) or was the old version of Syn fixing this for us ?

please see https://github.com/minkphp/driver-testsuite/blob/f038a1493dbb378b1fc2912a4806fe0117ca1c59/tests/Js/EventsTest.php#L107 test - int is passed

and how the places - keyXxx methods are annotated - https://github.com/minkphp/Mink/blob/852ce7865833bc482ff4ac9db6d7ed43c58ee4c3/src/Driver/DriverInterface.php#L525

the phpdoc was simply wrong before

@stof , I can confirm, that:

\Behat\Mink\Driver\DriverInterface::keyPress method PhpDoc has @param string|int $char ...;

implementation of that interface method \Behat\Mink\Driver\Selenium2Driver::keyPress is passing that $char parameter to the \Behat\Mink\Driver\Selenium2Driver::charToOptions method (changed below).

I agree with the made PhpDoc change, but I don't agree with the is_numeric change into is_int as this could be a BC break.

After a thorough investigation, I longer consider this a BC break and agree with a merge.

aik099 · 2022-10-20T07:08:48Z

src/Selenium2Driver.php

@@ -204,7 +204,7 @@ public static function getDefaultCapabilities()
    protected function withSyn()
    {
        $hasSyn = $this->wdSession->execute(array(
-            'script' => 'return typeof window["Syn"]!=="undefined" && typeof window["Syn"].trigger!=="undefined"',
+            'script' => 'return typeof window.syn !== \'undefined\' && typeof window.syn.trigger !== \'undefined\'',


Replacing typeof ... !== \'undefined\' constructs with ... !== undefined would also work.

done - https://github.com/minkphp/MinkSelenium2Driver/compare/5e4e4bda508ca409eed984686535f6bdce233d58..c2ec87dbf825fcccccf962dc3e525ca6621eb720

aik099 · 2022-10-20T07:18:57Z

src/Selenium2Driver.php

@@ -222,25 +222,29 @@ protected function withSyn()
    /**
     * Creates some options for key events
     *
-     * @param string $char     the character or code
-     * @param string $modifier one of 'shift', 'alt', 'ctrl' or 'meta'
+     * @param string|int  $char     the character or code


@stof , I can confirm, that:

\Behat\Mink\Driver\DriverInterface::keyPress method PhpDoc has @param string|int $char ...;

implementation of that interface method \Behat\Mink\Driver\Selenium2Driver::keyPress is passing that $char parameter to the \Behat\Mink\Driver\Selenium2Driver::charToOptions method (changed below).

I agree with the made PhpDoc change, but I don't agree with the is_numeric change into is_int as this could be a BC break.

aik099 · 2022-10-20T07:20:47Z

src/Selenium2Driver.php

+        if (is_int($char)) {
+            $charCode = $char;
+            $char = chr($charCode);
+        } else {
+            $charCode = ord($char);
        }


Please revert this code fragment, because it could potentially lead to the BC break when is_int($char) and is_numeric($char) yields different results.

@aik099 for ex. when 1 char/string is passed, I expect 1 to be pressed, is_numeric was wrong here, also all tests pass

That's an interesting observation. Never thought that number (e.g. 1, 7) could also be interpreted as a symbol and not a char code. Need to look closely at these tests you're talking about.

@aik099 please merge minkphp/driver-testsuite#61 test for it, also adds a test for capital letter key press

@stof , I've debugged the original (prior to this PR) version of the \Behat\Mink\Driver\Selenium2Driver::charToOptions method and the results are as follows:

When a letter is given as input, then it's converted into an ASCII code

When a number is given (as a string or as an integer) it's interpreted as an ASCII code itself

This behavior partially matches the DocBlock on the \Behat\Mink\Driver\DriverInterface::keyPress:

* @param string|int $char could be either char ('b') or char-code (98)

Due to the usage of the is_numeric function (in implementation), a digit from '0' to '9' passed in as a string was interpreted as an ASCII code as well, which is a bug.

To overcome this developers likely had to pass in the ASCII code of these digits. Tests for the such case were missing, but now they're added via minkphp/driver-testsuite#61 .

aik099 · 2022-11-18T10:22:13Z

@mvorisek , I've rerun the builds and they seem to pass with a new test.

mvorisek · 2022-11-18T10:43:33Z

@aik099 yes, please merge this PR as well and close the 3 mentioned #/issues in the description then.

aik099 · 2022-11-18T10:56:25Z

@aik099 yes, please merge this PR as well and close the 3 mentioned #/issues in the description then.

I'll wait for @stof to see if he has anything to add.

mvorisek · 2022-11-24T23:09:14Z

Hi @stof, this PR is approved but Alex wants your feedback as well, can you please give this PR a green light?

mvorisek · 2023-02-23T17:35:51Z

@stof @aik099 can this PR be merged? Thank you.

aik099 · 2023-02-24T16:10:12Z

@mvorisek , I guess you need to add a line to the changelog explaining the bug, that was fixed with this PR: the numbers from 0 to 9 passed as strings were treated as ASCII codes.

After that, I'll try to merge this PR.

mvorisek · 2023-02-25T22:00:43Z

@aik099 CHANGELOG added

aik099 · 2023-02-26T17:59:17Z

Merged. Thank you @mvorisek .

mvorisek · 2023-03-01T00:59:29Z

@aik099 Thank you, would it be possible to tag a new release please?

aik099 · 2023-05-11T07:49:16Z

How that would help you?

mvorisek · 2023-05-11T10:50:38Z

I no longer need it as I desided to fork this repo..

But I would like to get my changes still merged, especially #359. Thank you.

mvorisek force-pushed the upgrade_syn_015 branch 4 times, most recently from 6ffe75a to cfb81e9 Compare October 16, 2022 22:11

mvorisek force-pushed the upgrade_syn_015 branch from cfb81e9 to 2920fdc Compare October 16, 2022 22:32

mvorisek marked this pull request as ready for review October 16, 2022 22:32

mvorisek mentioned this pull request Oct 16, 2022

Fix obsolete event.which for key testing minkphp/driver-testsuite#58

Merged

mvorisek force-pushed the upgrade_syn_015 branch from 08c6a48 to 4bc7bc2 Compare October 17, 2022 11:24

stof reviewed Oct 17, 2022

View reviewed changes

mvorisek force-pushed the upgrade_syn_015 branch from 4bc7bc2 to 5e4e4bd Compare October 17, 2022 13:02

aik099 requested changes Oct 20, 2022

View reviewed changes

Upgrade JS syn to v0.15.0

c2ec87d

mvorisek force-pushed the upgrade_syn_015 branch from 5e4e4bd to c2ec87d Compare October 20, 2022 08:51

mvorisek mentioned this pull request Oct 23, 2022

Run tests also with Selenium 3.x #354

Closed

aik099 approved these changes Nov 18, 2022

View reviewed changes

add CHANGELOG

0405fe7

aik099 approved these changes Feb 26, 2023

View reviewed changes

aik099 merged commit affb7ea into minkphp:master Feb 26, 2023

mvorisek deleted the upgrade_syn_015 branch February 26, 2023 17:59

mvorisek mentioned this pull request Mar 1, 2023

Update Syn to 0.15.0 #333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade JS syn to v0.15.0 #358

Upgrade JS syn to v0.15.0 #358

mvorisek commented Oct 16, 2022 •

edited

Loading

codecov bot commented Oct 16, 2022 •

edited

Loading

stof Oct 17, 2022

mvorisek Oct 17, 2022

stof Oct 17, 2022

stof Oct 17, 2022

mvorisek Oct 17, 2022

mvorisek Oct 18, 2022

stof Oct 18, 2022

mvorisek Oct 18, 2022

aik099 Oct 20, 2022

aik099 Nov 18, 2022

aik099 Oct 20, 2022

mvorisek Oct 20, 2022 •

edited

Loading

aik099 Oct 20, 2022

aik099 Oct 20, 2022

mvorisek Oct 20, 2022

aik099 Oct 21, 2022

mvorisek Oct 25, 2022

aik099 Nov 18, 2022

aik099 commented Nov 18, 2022 •

edited

Loading

mvorisek commented Nov 18, 2022

aik099 commented Nov 18, 2022 •

edited

Loading

mvorisek commented Nov 24, 2022

mvorisek commented Feb 23, 2023

aik099 commented Feb 24, 2023 •

edited

Loading

mvorisek commented Feb 25, 2023

aik099 commented Feb 26, 2023

mvorisek commented Mar 1, 2023

aik099 commented May 11, 2023

mvorisek commented May 11, 2023 •

edited

Loading

Upgrade JS syn to v0.15.0 #358

Upgrade JS syn to v0.15.0 #358

Conversation

mvorisek commented Oct 16, 2022 • edited Loading

codecov bot commented Oct 16, 2022 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvorisek Oct 20, 2022 • 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

aik099 commented Nov 18, 2022 • edited Loading

mvorisek commented Nov 18, 2022

aik099 commented Nov 18, 2022 • edited Loading

mvorisek commented Nov 24, 2022

mvorisek commented Feb 23, 2023

aik099 commented Feb 24, 2023 • edited Loading

mvorisek commented Feb 25, 2023

aik099 commented Feb 26, 2023

mvorisek commented Mar 1, 2023

aik099 commented May 11, 2023

mvorisek commented May 11, 2023 • edited Loading

mvorisek commented Oct 16, 2022 •

edited

Loading

codecov bot commented Oct 16, 2022 •

edited

Loading

mvorisek Oct 20, 2022 •

edited

Loading

aik099 commented Nov 18, 2022 •

edited

Loading

aik099 commented Nov 18, 2022 •

edited

Loading

aik099 commented Feb 24, 2023 •

edited

Loading

mvorisek commented May 11, 2023 •

edited

Loading