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

Fix capybara-webkit driver to use synchronize correctly #403

Merged
merged 1 commit into from Oct 7, 2015
Merged

Fix capybara-webkit driver to use synchronize correctly #403

merged 1 commit into from Oct 7, 2015

Conversation

ryanong
Copy link
Contributor

@ryanong ryanong commented Aug 26, 2015

apparently you have to raise an error not just return false

@mikepack
Copy link
Collaborator

mikepack commented Oct 7, 2015

Thanks Ryan. This code makes sense and I understand where you're going with it, but I don't understand the problem. What was your expected behavior and what didn't work correctly? There are no new test cases added, so I'm not sure what problem you're solving.

@ryanong
Copy link
Contributor Author

ryanong commented Oct 7, 2015

Unfortunately the way this is tested ignores the actual capybara webkit code and would be quite difficult to actually do an integration test.

The problem is currently that the test will always return true no matter what because no error is raised. An error needs to be raised if the evaluated script is to be retried again.

@mikepack
Copy link
Collaborator

mikepack commented Oct 7, 2015

Got it. So, to clarify, you have a test that should fail, but is passing? This test is passing because the evaluation script does not rerun?

@mikepack
Copy link
Collaborator

mikepack commented Oct 7, 2015

Or, is the result of the test not being captured? It's neither passing nor failing. More like skipped. This is what I'd expect, not that the test actually passes.

@ryanong
Copy link
Contributor Author

ryanong commented Oct 7, 2015

more like skipped. The functionality that would make this fail is stubbed out and would only really fail if we added a 5 second sleep of some sort.

@mikepack
Copy link
Collaborator

mikepack commented Oct 7, 2015

Thanks Ryan!

mikepack added a commit that referenced this pull request Oct 7, 2015
Fix capybara-webkit driver to use synchronize correctly
@mikepack mikepack merged commit 3c30869 into jejacks0n:master Oct 7, 2015
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

2 participants