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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented Flask test client driver #300

Merged
merged 4 commits into from Apr 8, 2014
Merged

Implemented Flask test client driver #300

merged 4 commits into from Apr 8, 2014

Conversation

fgimian
Copy link
Contributor

@fgimian fgimian commented Mar 30, 2014

Hello there,

I've implemented a Flask test client driver heavily based on the provided Django driver. A lot of the code is shared between these modules, but I have kept them separate. Feel free to adjust this to taste 馃槃

I have replicated the Django tests and can confirm that all tests pass as shown below...

(splinter)fots@fotsies-ubprecise-01:~/splinter$ ./run_tests.py -w tests/test_flaskclient.py
................................................................................................................
----------------------------------------------------------------------
Ran 112 tests in 0.482s

OK

If you have any questions, please let me know 馃槃

Cheers
Fotis

for key, value in cookie.items():
self._cookies.set_cookie('localhost', key, value)
return
for key, value in cookies.items():
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be inside an else block, because if cookies is an empty list, it's going to jump to this for line, and raise an AttributeError because list objects do not have .items() method.

Copy link
Member

Choose a reason for hiding this comment

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

I see that webdriver's cookie manager has the same issue. You can skip this if you want and then we work on both issues together in a different PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted, although I think that the main issue is the indent of the return statement. The return statement should be un-indented by one level because at present it only processes the first set of cookies in the list of dicts. In addition, moving the second for loop into an else block would be great for clarity.

e.g.

>>> def add(cookies):
...     if isinstance(cookies, list):
...         for cookie in cookies:
...             for key, value in cookie.items():
...                 print '%s: %s' % (key, value)
...             return
...
>>> add([
...     {'a': 'a1', 'b': 'b1'},
...     {'c': 'c1', 'd': 'd1'},
... ])
a: a1
b: b1
>>>

However as you said, this is an issue across a few of the drivers which would probably best be fixed with a single pull request 馃槃

@hltbra
Copy link
Member

hltbra commented Mar 30, 2014

Thanks for the pull request, it looks very good! I am going to wait for more reviewers.

@fgimian
Copy link
Contributor Author

fgimian commented Mar 31, 2014

Can't thank you enough for the detailed review, really appreciate it!! 馃憤

Cheers
Fotis

@fgimian
Copy link
Contributor Author

fgimian commented Apr 5, 2014

This pull request has now been updated for Python 3 compatibility and is ready for merge 馃槃

Cheers
Fotis

@andrewsmedina
Copy link
Member

I will create issues for the things highlighted by @hltbra. Then I will merge this PR.

@fgimian
Copy link
Contributor Author

fgimian commented Apr 8, 2014

Sounds great! Please let me know if you need any help mate 馃槃

@andrewsmedina
Copy link
Member

@fgimian and @hltbra thank you!

andrewsmedina added a commit that referenced this pull request Apr 8, 2014
Implemented Flask test client driver
@andrewsmedina andrewsmedina merged commit bff8698 into cobrateam:master Apr 8, 2014
@fgimian fgimian deleted the flask_driver branch April 8, 2014 03:44
@fgimian
Copy link
Contributor Author

fgimian commented Apr 8, 2014

Thank you so much, you've made my day! 馃槃

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