search

twalpole / apparition

Capybara driver for Chrome using CDP
MIT License
363 stars 46 forks source link

Add Capybara::Apparition::NetworkTraffic::Request#finished? #49

Open dkniffin opened 4 years ago

dkniffin commented 4 years ago

I would like to add this method, which returns whether the request is completed or still pending. I have no idea if this is the correct implementation for it. In fact, testing it on my app, there are requests where this is not true, even after a long period of time. If someone could give me feedback on this, that would be awesome!

dkniffin commented 4 years ago

I just pushed a new version of this change that uses the Network.loadingFinished event to determine this instead.

twalpole commented 4 years ago

Does the new change work with the requests you said the other wasn’t?

dkniffin commented 4 years ago

@twalpole I'm actually not sure. I didn't test this with our app. I would definitely want to add unit tests into this PR, but I wanted to make sure you were okay with the approach and willing to accept it before I spent time on that.

twalpole commented 4 years ago

I’m willing if it works.

dkniffin commented 4 years ago

@twalpole Hmm. Looking at the tests now, I'm not actually sure how I would test this. It looks like there's not a lot of unit tests. How would you suggest I add a test for it?

twalpole commented 4 years ago

Apparition runs it's test suite plus the whole of the Capybara test suite. I would recommend testing it by making requests to endpoints in the test app (new or existing ones) that delay the response a couple of seconds, verifying that the response isn't completed and then after the expected delay verifying the response is completed - similar to what the network traffic tests do - https://github.com/twalpole/apparition/blob/master/spec/integration/driver_spec.rb#L706

Capybara provided portion of the test suite is at https://github.com/teamcapybara/capybara/tree/master/lib/capybara/spec

dkniffin commented 4 years ago

@twalpole Alright, added a pretty simple test. Let me know if that is sufficient or not.

twalpole commented 4 years ago

@dkniffin The test you modified is now failing - I'm not sure if it's because of your change or not. Please add a new test that specifically tests the finished? method rather than merging into an existing test so it's clear which part isn't working.

twalpole commented 4 years ago

@dkniffin Any update on changing this to be its own new test rather than hacking into an existing one?

dkniffin commented 4 years ago

@twalpole sorry, I've been busy. Thank you for the reminder. I'll try to make time to finish it up sometime this week.

dkniffin commented 4 years ago

I finally had a few minutes to take a look at this. I just split it to it's own spec. We'll see if that resolved the failure, or which of the tests is failing now.

twalpole commented 4 years ago

@dkniffin The test appears to fail