Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error with chromedriver 2.46 #5401

Closed
JorisVanEijden opened this issue Feb 20, 2019 · 16 comments
Closed

Error with chromedriver 2.46 #5401

JorisVanEijden opened this issue Feb 20, 2019 · 16 comments

Comments

@JorisVanEijden
Copy link

JorisVanEijden commented Feb 20, 2019

What are you trying to achieve?

Use chromedriver to run tests.

What do you get instead?

[Facebook\WebDriver\Exception\UnrecognizedExceptionException] invalid argument: invalid 'expiry'
(Session info: chrome=72.0.3626.81)
(Driver info: chromedriver=2.46.628388 (4a34a70827ac54148e092aafb70504c4ea7ae926),platform=Linux 4.9.125-linuxkit x86_64)

#1  /var/www/html/vendor/facebook/webdriver/lib/Exception/WebDriverException.php:158
#2  /var/www/html/vendor/facebook/webdriver/lib/Remote/HttpCommandExecutor.php:326
#3  /var/www/html/vendor/facebook/webdriver/lib/Remote/RemoteWebDriver.php:547
#4  /var/www/html/vendor/facebook/webdriver/lib/Remote/RemoteExecuteMethod.php:40
#5  /var/www/html/vendor/facebook/webdriver/lib/WebDriverOptions.php:55
#6  Codeception\Module\WebDriver->loadSessionSnapshot

Details

  • Codeception version: 2.5.3
  • PHP Version: 7.1
  • Operating System: Any
  • Installation type: Composer
  • Suite configuration:
modules:
    enabled:
        -
          WebDriver:
            browser: chrome
            capabilities:
              javascriptEnabled: true
              resourceTimeout: 300000
            host: %WEBDRIVER_HOST%
            url: %WEBSITE_URL%

Chromedriver 2.45 works fine.
In 2.46 they added extra cookie validation (https://chromium-review.googlesource.com/c/chromium/src/+/1366925)

@Naktibalda
Copy link
Member

Isn't it a bug of Chromedriver then?
I've seen this issue few weeks ago and it looked like Chromedriver broke setCookie in legacy mode completely, but facebook/php-webdriver library does not support W3C mode yet.

JorisVanEijden pushed a commit to JorisVanEijden/Codeception that referenced this issue Feb 21, 2019
…not null which fails in chromedriver 2.46
JorisVanEijden pushed a commit to JorisVanEijden/Codeception that referenced this issue Feb 21, 2019
@OneEyedSpaceFish
Copy link
Contributor

@JorisVanEijden
Imo, if this is a bug with Chromedriver, then it's worth raising an issue / PR with them instead of adding a workaround solution in CC.

@JorisVanEijden
Copy link
Author

JorisVanEijden commented Feb 21, 2019

I don't think it's a bug in Chromedriver to state that null is an invalid value for those cookie fields.
From https://github.com/SeleniumHQ/selenium/wiki/JsonWireProtocol I would also expect null to be an invalid value.
https://w3c.github.io/webdriver/#add-cookie basically says the same.

I could raise a PR with Facebook webdriver to refuse cookies with null values. But then you still need this PR for Codeception.

Edit: Facebook php-webdriver issue: php-webdriver/php-webdriver#626

@JorisVanEijden
Copy link
Author

I've created php-webdriver/php-webdriver#627 to stop php-webdriver from passing the invalid cookie values it gets from Codeception to chromedriver.

@jeckel
Copy link

jeckel commented Mar 17, 2019

I'm not sure I understand this issue...

Tell me if I'm wrong :

  • chrome updated his setCookie and broke legacy compatibility (something linked to default values in setCookie ? like path or expiry when this values are null)
  • webdriver (maintained by facebook) haven't been updated to handle this change correctly, and in some cases, it still call setCookie with null value for some parameters
  • codeception, when using webdriver don't set this values and expect WebDriver to handle them correctly with some defaults

Am I correct ?

@Kender2
Copy link

Kender2 commented Mar 17, 2019

  • cookie specs say values should not be null and chromedriver 2.46 started enforcing that
  • webdriver defaults to null cookie values for missing values
  • codeception does not supply defaults for cookie values

So yes.

@jeckel
Copy link

jeckel commented Mar 17, 2019

@Kender2 Great ! thanks

Then what's next ? this commit from @JorisVanEijden seems good to me, is there anything against adding default values in CC ?

JorisVanEijden@ec2fc73

@reinholdfuereder
Copy link
Contributor

I tend to also see it like @jeckel and hope for a new (minor?) Codeception version release with this fix/workaround -- especially considering the lack of responses/actions on @JorisVanEijden commitment in
php-webdriver/php-webdriver#626 and/or php-webdriver/php-webdriver#627

  • Would it even hurt if Codeception sets this default cookie values, once facebook/php-webdriver would have a fix too?

@Naktibalda
Copy link
Member

facebook/php-webdriver looks completely unmaintained since June last year.

I will merge workaroud before next release.

@reinholdfuereder
Copy link
Contributor

@Naktibalda Thanks (in advance)!

Please note that I just assume that @JorisVanEijden fix solves the problem as I have not tested it at all, and neither have expert knowledge; but the changes definitely look good with respect to the failure symptoms ;-)


Rather unrelated, except that it is about "facebook/php-webdriver"

The situation around "facebook/php-webdriver" especially with respect to Firefox and W3C protocol actually makes me a little bit nervous too; although there is (or was) quite some work done, I am not sure how active/promising it is.

@jeckel
Copy link

jeckel commented Mar 20, 2019

I tested @JorisVanEijden fix on my side and it solved my issue

@JorisVanEijden
Copy link
Author

I'm sorry, I have not continued with this after patching php-webdriver.

The workaround composer.json I use:

{
  "require-dev": {
    "codeception/codeception": "^2.0",
    "cweagans/composer-patches": "^1.0",
  },
  "extra": {
    "patches": {
      "facebook/webdriver": {
        "issue_626": "https://patch-diff.githubusercontent.com/raw/facebook/php-webdriver/pull/627.patch"
      }
    }
  }
}

I remember running into some strange issues with PR #5404 but I was unable to figure out if that was due to the PR, php-webdriver, chromedriver or even my test suite :(
Please test carefully.

@jeckel
Copy link

jeckel commented Mar 20, 2019

On my side, my test suite is really simple, then it might not be enough to validate this PR.

By the way, thanks for the tips to patch webdriver.

@Naktibalda
Copy link
Member

Fixed in Codeception 2.5.5 which was released today.

@JorisVanEijden
Copy link
Author

JorisVanEijden commented Apr 16, 2019

Ok, I figured out what the issue was.
The array_filter removes the value entry from the cookie when the value is an empty string or 0 (zero).
This will fail because value is a required field.

Created #5470 to fix it.

@zfz1120
Copy link

zfz1120 commented Jul 31, 2019

expiry 格式变了。。。把float该为int即可。。。亲测

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants