Skip to content

Conversation

@coraxster
Copy link
Contributor

add support for selenium(also selenium grid)

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add some (functional?) tests please?

use Facebook\WebDriver\Remote\RemoteWebDriver;
use Facebook\WebDriver\WebDriver;
use \Facebook\WebDriver\WebDriverCapabilities;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra line to remove

use Facebook\WebDriver\Remote\DesiredCapabilities;
use Facebook\WebDriver\Remote\RemoteWebDriver;
use Facebook\WebDriver\WebDriver;
use \Facebook\WebDriver\WebDriverCapabilities;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial backslack must also be removed

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but can you run PHP CS Fixer to fix the CS please?

use PHPUnit\Framework\TestCase;

/**
* @author Kévin Dunglas <dunglas@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be changed I guess!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@dunglas dunglas Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It defines the copyright, not the author! You can update this line safely, PHP CS Fixer will not complain.

@coraxster
Copy link
Contributor Author

ci passed

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last minor change then 👍

private $host;
private $capabilities;

public function __construct(?string $host = null, ?WebDriverCapabilities $capabilities = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be string $host = self::DEFAULT_HOST IIRC.


final class SeleniumManager implements BrowserManagerInterface
{
private const DEFAULT_HOST = 'http://127.0.0.1:4444/wd/hub';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not even sure we need this constant, can be the default value in the constructor directly.

@coraxster
Copy link
Contributor Author

all fixed :)

}

private function getDefaultArguments(): array
public function getDefaultArguments(): array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? If it is just for https://github.com/dunglas/panthere/pull/15/files#diff-0b7a157b54171e8b04337166e6da76d3R47, it is not worth it, just copy them.

@dunglas dunglas merged commit e009954 into symfony:master Jun 12, 2018
@dunglas
Copy link
Member

dunglas commented Jun 12, 2018

Thank you very much @coraxster! You you mind updating the readme to explain how to use Selenium?

@coraxster
Copy link
Contributor Author

Thank you for help :)
No problem.

dunglas pushed a commit that referenced this pull request Dec 28, 2020
* Selenium manager

* Selenium manager create method

* fix cs

* simple test

* fix selenium manager test

* fix travis ci test, fix cs

* fix cs

* fix cs, refactor constructor

* fix constructor

* fix test
nicolas-grekas pushed a commit that referenced this pull request Jan 18, 2021
* Selenium manager

* Selenium manager create method

* fix cs

* simple test

* fix selenium manager test

* fix travis ci test, fix cs

* fix cs

* fix cs, refactor constructor

* fix constructor

* fix test
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

Successfully merging this pull request may close these issues.

2 participants