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

made refactoring and added test for browser_instance fixture #7

Merged
merged 13 commits into from
Sep 6, 2014

Conversation

amakhnach
Copy link
Contributor

Made changes related to #5

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b06a0e9 on issue5 into * on master*.

@@ -176,16 +176,22 @@ def browser_instance(
}, **splinter_firefox_profile_preferences)
if splinter_driver_kwargs:
kwargs.update(splinter_driver_kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

let's rename the fixture to reflect what it returns
also i think we should make it session scoped, as patching happens there and we return a function, so it's safe and efficient to make it session scoped

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I agree. But which name? get_browser_instance? browser_get_instance?

Copy link
Member

Choose a reason for hiding this comment

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

browser_instance_getter

On 28 June 2014 23:11, Andrey Makhnach notifications@github.com wrote:

In pytest_splinter/plugin.py:

@@ -176,16 +176,22 @@ def browser_instance(
}, **splinter_firefox_profile_preferences)
if splinter_driver_kwargs:
kwargs.update(splinter_driver_kwargs)

Okay, I agree. But which name? get_browser_instance? browser_get_instance?


Reply to this email directly or view it on GitHub
https://github.com/paylogic/pytest-splinter/pull/7/files#r14326262.

Anatoly Bubenkov

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 838b9a7 on issue5 into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7528079 on issue5 into * on master*.

@@ -176,16 +176,22 @@ def browser_instance(
}, **splinter_firefox_profile_preferences)
Copy link
Member

Choose a reason for hiding this comment

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

docstring is not updated

bubenkoff added a commit that referenced this pull request Sep 6, 2014
made refactoring and added test for browser_instance fixture
@bubenkoff bubenkoff merged commit 1c982da into master Sep 6, 2014
@bubenkoff bubenkoff deleted the issue5 branch September 6, 2014 03:30
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.83%) when pulling f136da1 on issue5 into c406f6f on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.83%) when pulling f136da1 on issue5 into c406f6f on master.

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.

None yet

3 participants