add option --splinter-webdriver-executable for phantomjs and chrome #24

sureshvv · 2015-04-13T04:16:45Z

No description provided.

Conflicts: CHANGES.rst pytest_splinter/plugin.py

coveralls · 2015-04-13T04:20:19Z

Coverage increased (+0.37%) to 90.52% when pulling 4d31822 on sureshvv:splinter-webdriver-executable into fa4dc1e on pytest-dev:master.

sureshvv · 2015-04-14T09:01:46Z

use keyword style for passing such long list of arguments

Are you sure this is a good idea? The arguments already have long names and they are fixtures so cannot make then short, We will end up invoking it like this:

get_args(splinter_webdriver_executavle=splinter_webdriver_executable
splinter_webdriver_executavle=splinter_webdriver_executable
splinter_webdriver_executavle=splinter_webdriver_executable
splinter_webdriver_executavle=splinter_webdriver_executable
splinter_webdriver_executavle=splinter_webdriver_executable
splinter_webdriver_executavle=splinter_webdriver_executable
splinter_webdriver_executavle=splinter_webdriver_executable
splinter_webdriver_executavle=splinter_webdriver_executable
),

Since this is an internal function anyway, I think it is better to keeo it as it is.

bubenkoff · 2015-04-14T09:09:07Z

nobody stops you to rename the parameters in get_args function so it will
look like:

get_args(executable=splinter_webdriver_executable,
firefox_profile_directory=splinter_firefox_profile_directory...
which is still much better than positional arguments

On 14 April 2015 at 11:01, sureshvv notifications@github.com wrote:

use keyword style for passing such long list of arguments

Are you sure this is a good idea? The arguments already have long names
and they are fixtures so cannot make then short, We will end up invoking it
like this:

get_args(splinter_webdriver_executavle=splinter_webdriver_executable
splinter_webdriver_executavle=splinter_webdriver_executable
splinter_webdriver_executavle=splinter_webdriver_executable
splinter_webdriver_executavle=splinter_webdriver_executable
splinter_webdriver_executavle=splinter_webdriver_executable
splinter_webdriver_executavle=splinter_webdriver_executable
splinter_webdriver_executavle=splinter_webdriver_executable
splinter_webdriver_executavle=splinter_webdriver_executable
),

Since this is an internal function anyway, I think it is better to keeo it
as it is.

—
Reply to this email directly or view it on GitHub
#24 (comment)
.

Anatoly Bubenkov

sureshvv · 2015-04-14T09:35:56Z

But then the code inside get_args will have to change, and it will become confusing to people who know/follow the code. Currently git diff now shows the same block of code extracted and neatly moved up into a separate function.

While I agree that the keyword style arguments will make the test look prettier, I am not so sure that it is an overall improvement.

bubenkoff · 2015-04-14T09:42:24Z

git diff will also show you renamed arguments, which is pretty readable,
don't worry too much
i worry more about how it looks after refactoring

On 14 April 2015 at 11:35, sureshvv notifications@github.com wrote:

But then the code inside get_args will have to change, and it will become
confusing to people who know/follow the code. Currently git diff now shows
the same block of code extracted and neatly moved up into a separate
function.

While I agree that the keyword style arguments will make the test look
prettier, I am not so sure that it is an overall improvement.

—
Reply to this email directly or view it on GitHub
#24 (comment)
.

Anatoly Bubenkov

…-webdriver-executable Conflicts: pytest_splinter/plugin.py tests/test_plugin.py

coveralls · 2015-04-14T10:07:11Z

Coverage increased (+0.85%) to 91.0% when pulling 97deda8 on sureshvv:splinter-webdriver-executable into cb4f566 on pytest-dev:master.

coveralls · 2015-04-14T11:13:32Z

Coverage increased (+0.85%) to 91.0% when pulling 5216103 on sureshvv:splinter-webdriver-executable into cb4f566 on pytest-dev:master.

bubenkoff · 2015-04-14T11:18:07Z

pytest_splinter/plugin.py

did you forget to pass as kwargs?

Don't follow your question.

get_args returns a dict which is passed as kwargs to the Browser() constructor. get_args has keyword arguments.

call get_args like:

get_args(driver=splinter_webdriver,....

coveralls · 2015-04-14T11:55:58Z

Coverage increased (+0.85%) to 91.0% when pulling 60f321c on sureshvv:splinter-webdriver-executable into cb4f566 on pytest-dev:master.

add option --splinter-webdriver-executable for phantomjs and chrome

bubenkoff · 2015-04-14T12:18:06Z

thanks!
I'll fix those minor pep257 issues myself
in general it's good idea to set up your editor the way it checks that

bubenkoff · 2015-04-14T14:13:08Z

1.3.5 is out

sureshvv · 2015-04-20T19:47:47Z

Thank you. Is pep257 checking not part of tox tests? I thought I addressed all the issues.

bubenkoff · 2015-04-20T19:48:31Z

now it is :)

On 20 April 2015 at 21:47, sureshvv notifications@github.com wrote:

Thank you. Is pep257 checking not part of tox tests? I thought I addressed
all the issues.

—
Reply to this email directly or view it on GitHub
#24 (comment)
.

Anatoly Bubenkov

bh · 2015-04-21T11:00:23Z

I have an issue after this pull request. My phantomjs binary is in ~/.virtualenvs/mx/bin/ (installed via nodeenv). After update to current version of pytest-splinter, I always get the following exception:

    def start(self):
        """
            Starts PhantomJS with GhostDriver.

            :Exceptions:
             - WebDriverException : Raised either when it can't start the service
               or when it can't connect to the service
            """
        try:
            self.process = subprocess.Popen(self.service_args, stdin=subprocess.PIPE,
                                            close_fds=platform.system() != 'Windows',
                                            stdout=self._log, stderr=self._log)

        except Exception as e:
>           raise WebDriverException("Unable to start phantomjs with ghostdriver.", e)
E           selenium.common.exceptions.WebDriverException: Message: Unable to start phantomjs with ghostdriver.
E           Screenshot: available via screen

I fixed this with this workaround in my conftest.py:

@pytest.fixture(scope='session')
def splinter_webdriver_executable(request):
    return distutils.spawn.find_executable(request.config.option.splinter_webdriver)

bubenkoff · 2015-04-21T11:08:03Z

thanks for noticing!
please check out 1.3.6 which is just out

sureshvv · 2015-04-21T11:43:06Z

--splinter-webdriver-executable=$HOME/.virtualenvs/mx/bin should help you. But not sure why this pull request broke it. Was it working before?

bubenkoff · 2015-04-21T11:44:43Z

look at the last commit :)

sureshvv · 2015-04-21T11:48:37Z

aah.. coincidentally named fixtures :)

bh · 2015-04-21T11:51:02Z

On 04/21/2015 01:43 PM, sureshvv wrote:

--splinter-webdriver-executable=$HOME/.virtualenvs/mx/bin should help
you. But not sure why this pull request broke it. Was it working before?

—
Reply to this email directly or view it on GitHub
#24 (comment).

Of couse. It should has to be possible to find the binary in $PATH. I
don't want to be too explicitly ;)

bh · 2015-04-21T12:11:18Z

On 04/21/2015 01:50 PM, Benjamin Hedrich wrote:

On 04/21/2015 01:43 PM, sureshvv wrote:

--splinter-webdriver-executable=$HOME/.virtualenvs/mx/bin should help
you. But not sure why this pull request broke it. Was it working before?

—
Reply to this email directly or view it on GitHub
#24 (comment).

Of couse. It should has to be possible to find the binary in $PATH. I
don't want to be too explicitly ;)

py.test --cov=mx --cov-report=term-missing --cov-report html mx/

test session starts

platform linux -- Python 3.4.3 -- py-1.4.26 -- pytest-2.7.0
rootdir: /home/bhe/devel/mx/src, inifile: pytest.ini
plugins: splinter, bdd, cov, django, ipdb, rerunfailures, xdist
collected 375 items

mx/account/tests/acceptance/test_email_confirmation.py ..
[...]

With 1.3.6 the issue seems to be resolved! Thank you!

sureshvv · 2015-04-21T17:28:48Z

Actually I think it is better to raise an Exception if executable not found rather than quietly ignore the option,

blueyed · 2015-04-21T17:52:21Z

btw: while looking into why the error from subprocess.Popen (the exception) is not
displayed, I've found a bug about the usage of WebDriverException in Selenium's
driver. See SeleniumHQ/selenium#475.

blueyed · 2015-04-21T17:58:36Z

And I agree with @sureshvv: it's an error and should not pass silently.

bubenkoff · 2015-04-21T18:26:42Z

@sureshvv but where do you see that we silently ignore the option?

sureshvv · 2015-04-21T22:04:47Z

@bubenkoff @blueyed Sorry.. I was reading too fast :-) It is fine as it is

Suresh V added 7 commits April 7, 2015 16:19

Make browser_instance_getter session scoped

3e36ec4

Make browser_instance_getter session scoped

f5a1d4b

Merge branch 'master' of github.com:pytest-dev/pytest-splinter

4155b09

Conflicts: CHANGES.rst pytest_splinter/plugin.py

command line option --splinter-webdriver-executable for chrome/phantomjs

b761217

Improved test coverage

740be29

add option --splinter-webdriver-executable for phantomjs and chrome

f45fbfb

add option --splinter-webdriver-executable for phantomjs and chrome

4d31822

Merge branch 'master' of github.com:pytest-dev/pytest-splinter

38f1d70

Suresh V added 2 commits April 14, 2015 15:26

prefer keyword arguments

bea9420

Merge commit 'bea9420cb5a66caa7c3d65d3e382ad1cc6c2398e' into splinter…

97deda8

…-webdriver-executable Conflicts: pytest_splinter/plugin.py tests/test_plugin.py

Suresh V added 2 commits April 14, 2015 16:13

code review comments

e79671b

use imperative mood

5216103

bubenkoff reviewed Apr 14, 2015
View reviewed changes

help it land

60f321c

bubenkoff added a commit that referenced this pull request Apr 14, 2015

Merge pull request #24 from sureshvv/splinter-webdriver-executable

e556275

add option --splinter-webdriver-executable for phantomjs and chrome

bubenkoff merged commit e556275 into pytest-dev:master Apr 14, 2015

sureshvv deleted the splinter-webdriver-executable branch April 21, 2015 22:05

add option --splinter-webdriver-executable for phantomjs and chrome #24

add option --splinter-webdriver-executable for phantomjs and chrome #24

Uh oh!

Conversation

sureshvv commented Apr 13, 2015

Uh oh!

coveralls commented Apr 13, 2015

Uh oh!

sureshvv commented Apr 14, 2015

Uh oh!

bubenkoff commented Apr 14, 2015

Uh oh!

sureshvv commented Apr 14, 2015

Uh oh!

bubenkoff commented Apr 14, 2015

Uh oh!

coveralls commented Apr 14, 2015

Uh oh!

coveralls commented Apr 14, 2015

Uh oh!

bubenkoff Apr 14, 2015

Choose a reason for hiding this comment

Uh oh!

sureshvv Apr 14, 2015

Choose a reason for hiding this comment

Uh oh!

bubenkoff Apr 14, 2015

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 14, 2015

Uh oh!

bubenkoff commented Apr 14, 2015

Uh oh!

bubenkoff commented Apr 14, 2015

Uh oh!

sureshvv commented Apr 20, 2015

Uh oh!

bubenkoff commented Apr 20, 2015

Uh oh!

bh commented Apr 21, 2015

Uh oh!

bubenkoff commented Apr 21, 2015

Uh oh!

sureshvv commented Apr 21, 2015

Uh oh!

bubenkoff commented Apr 21, 2015

Uh oh!

sureshvv commented Apr 21, 2015

Uh oh!

bh commented Apr 21, 2015

Uh oh!

bh commented Apr 21, 2015

py.test --cov=mx --cov-report=term-missing --cov-report html mx/

test session starts

Uh oh!

sureshvv commented Apr 21, 2015

Uh oh!

blueyed commented Apr 21, 2015

Uh oh!

blueyed commented Apr 21, 2015

Uh oh!

bubenkoff commented Apr 21, 2015

Uh oh!

sureshvv commented Apr 21, 2015

Uh oh!

Uh oh!