-
Notifications
You must be signed in to change notification settings - Fork 50
Handle urlerror in make screenshot on failure #17
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
Handle urlerror in make screenshot on failure #17
Conversation
pytest_splinter/plugin.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if you need logging, and if you do, then it definitely should be not info but debug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't / should not affect production systems / logs, but is used only during tests - and in that case it's a good/useful information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, then modernize it - use .format
I've pushed a new commit, and added a test. Is it possible for the test to not spawn a browser window, like with the other tests in tests/test_screenshot.py? |
4e688ef
to
c5cb90d
Compare
That wasn't trivial - I hope I got it right?! Let me know if this is good, and I'll squash-merge it as a single commit into master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertion should be moved out inside of the 'root' test, then it will be ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried that first, but it did not work - the warning gets done in the inner test.
I've committed it in 008728f, and added an sentence to the doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please change back, it decreases the coverage, also the readability - those multiple returns in a single function
what's your username on pypi? |
actually i've just found out that test is a complete false-positive thing |
i think i'll fix it myself |
ok i fixed it |
1.2.10 is out |
Thanks for fixing (and releasing) it, and sorry for my oversights. |
Fixes #16.