panther icon indicating copy to clipboard operation
panther copied to clipboard

createPantherClient improvement

Open shadowc opened this issue 4 years ago • 16 comments

Adds ability to pass browser arguments to the client when a Browser has been specified in the options.

According to what we were able to discuss in #428 , there isn't a clear way to configure a browser window size for the browser (assuming we want to test in interactive mode) from createPantherClient.

I commented the following in the issue, looking for answers to this:

It would appear by looking at the source code (if I'm not wrong) that if
I call Client::createChromeClient I won't have started a
WebServer (which I guess it's fine in my case, but not ALL cases) and I
won't boot a kernel. What implications does this have for
the test case?

Regardless of the implications, I think this is probably a good addition: We can reach firefox/chrome options from createPantherClient instead of directly.

Since browser arguments are specific to browser type, I've added a check to nullify this argument IF a browser hasn't been specified by the user. I wonder if this is needed? If I pass a wrong argument to a browser, I suppose the driver will just ignore it and move on.

Let me know if this is a sensible addition or if there is a better way to handle this.

shadowc avatar Mar 02 '21 19:03 shadowc

Fixes #428

shadowc avatar Mar 03 '21 00:03 shadowc

Isn't this change a BC break?

dunglas avatar Mar 03 '21 06:03 dunglas

@dunglas since the new argument is optional and is introduced at the end of the argument list, it should not break anything.

nusje2000 avatar Mar 03 '21 07:03 nusje2000

It will. This is a protected method that can be overrode. https://symfony.com/doc/current/contributing/code/bc.html#changing-classes

dunglas avatar Mar 03 '21 07:03 dunglas

Will fix soon!

shadowc avatar Mar 03 '21 16:03 shadowc

It looks like tests are failing on Ubuntu becuase Chrome Driver crashes, but my local tests in Ubuntu 20.04 with PHP 7.4 are passing.

shadowc avatar Mar 04 '21 18:03 shadowc

I've pushed the approach proposed by @nusje2000 . What do you all think?

shadowc avatar Mar 05 '21 20:03 shadowc

Any idea why tests are failing left right and center with this error message?

) Symfony\Component\Panther\Tests\AssertionsTest::testDomCrawlerAssertions with data set "Panther" (array('Symfony\Component\Panther\Tes...nsTest', 'createPantherClient'), 'Symfony\Component\Panther\Client')
Facebook\WebDriver\Exception\UnknownErrorException: unknown error: Chrome failed to start: exited abnormally.
  (unknown error: DevToolsActivePort file doesn't exist)
  (The process started from chrome location /usr/bin/google-chrome is no longer running, so ChromeDriver is assuming that Chrome has crashed.)

(All the tests pass locally in my Ubuntu 20.04 btw)

shadowc avatar Mar 05 '21 20:03 shadowc

Alright. One last change. Defaulting browser_arguments to null made all the tests pass! Now also triggering a warning when $options['browser_arguments'] is not an array!

shadowc avatar Mar 07 '21 01:03 shadowc

Hi @shadowc! Gentle ping :) Do you still have the time to work on this PR or do you want someone else to try to finish it? Thanks!

dunglas avatar Jul 12 '21 16:07 dunglas

I'm so sorry for disappearing! There have been many IRL events that prevented me from doing ANY side work. I WANT to get back to this this weekend if that's ok with you!

shadowc avatar Jul 12 '21 17:07 shadowc

No worries, it's perfectly OK to have more important things to do. We aren't paid for this, after all!

dunglas avatar Jul 12 '21 19:07 dunglas

Hey, @dunglas ! So now I've corrected the file (also fixed conflicts with latest main) and wrote some tests, although I'm not sure if those are done in a correct way.

I am getting a timeout on PHP 8.0 on Mac which I'm not 100% sure it's unstable or a mistake on the way I'm deleting the client from one test to the next.

shadowc avatar Jul 18 '21 22:07 shadowc

Ping @dunglas ! So some unrelated test is failing due to something I cannot really understand (looks like an artifact on the github action? I don't have a Mac unfortunately). Let me know if I can do something this weekend to help with it (have my test broken anything?). (Also my tests might not be what you want/expect, so you can give me some feedback on that as well) :)

shadowc avatar Aug 02 '21 18:08 shadowc

Should I Ping again? I assume this PR is still relevant?

shadowc avatar Feb 12 '22 14:02 shadowc

This PR is green and ready for review.

shadowc avatar Feb 16 '22 18:02 shadowc