search

mantoni / min-webdriver

Pipe scripts to browsers using the Selenium WebDriver protocol
MIT License
25 stars 11 forks source link

Improved timeout configuration and periodically flush logs #17

Closed valotas closed 6 years ago

valotas commented 7 years ago

Replaced the async_script with all possible available timeouts defined at https://www.w3.org/TR/webdriver/#set-timeouts. I am not sure if I understood everything right but it looks like async_script is not supported?

On top of that I've introduced periodic log flushing.

npm test is green :) but I'll test this next week on a project once I'll be back from vacations. Till then, let me know if you see anything obviously wrong.

valotas commented 7 years ago

Something I forgot. I do not know if you want to introduce a package-lock.json, but since I was not sure, I git-ignored it.

mantoni commented 7 years ago

Thank you for the pull request! The flushing logic is a very good improvement :+1:

Can you explain why / in which scenario it's useful to configure the new timeouts? I'm not fully convinced they're actually needed. You removed the documentation explaining when to configure the previous "timeout" option. I'd like to keep that and also have some explanation about the other two timeout options.

Would it be possible to implement it in a backward compatible way? Like checking for the "timeouts" object, falling back to the "timeout" property. Otherwise this is a breaking change and I'd have to release it as a major :thinking:

mantoni commented 7 years ago

Ah, I think I just now understood how you're doing it. It's a generic "configure whatever timeout you want" solution now. You replaced the async_script with the script timeout. Can you elaborate why this change is needed?

valotas commented 7 years ago

The flush was your idea and pretty much your implementation.

Regarding the timeouts, I am having problems with the chrome driver and tests running for more than 10s. I am not completely sure though, so I just need a way to test different timeouts. I took a look at the docs and saw that the supported ones are script, pageLoad and implicit which can be configured with at /session/{session id}/timeouts endpoint. Didn't find async_script anywhere, so I assumed that you meant script there, that is why I used that.

The change should be backwards compatible if my assumption is right: As soon as you define a timeouts it will be used. If not I create a timeouts object using the given timeout attribute.

That said and if my assumption is correct, why did the async_script work in the first place? I can make some test the coming week and get back to you. Then I will also update the docs accordingly.

Apart from that, do you see anything else that is wrong?

mantoni commented 7 years ago

Okay, I googled it again. I was coding this against the spec from SeleniumHQ. There are two timeouts, one for asynchronous scripts (taking a callback) and one for synchronous ones (using the return value). And since we're making an asynchronous call to fetch the logs, that is the timeout to configure. See https://github.com/SeleniumHQ/selenium/wiki/JsonWireProtocol#sessionsessionidtimeoutsasync_script.

Regarding backward compatibility, you're right. I didn't read carefully. However, we're loosing the default value for the script timeout if timeouts is specified, right?

valotas commented 7 years ago

Yes. The idea is that if you want to set a timeout with the timeouts attribute you should define what exactly you want. Thinking about it again though, maybe it is better to "blindly" use the timeout to set all possible timeouts. That will keep the driver min, will cover most cases and if the need comes, it will be easy to change it in order to support multiple timeouts.

Regarding the async_script timeout, I am still not sure if the script one "covers" it. Shall I set both of them or go with the webdriver spec and wait for possiblem problems?

valotas commented 7 years ago

I had a look at it again. The solution works in terms that the timeouts are getting updated. I do not know how to test the difference of timeout.scripts and timeout.async_script. Any idea?

mantoni commented 7 years ago

Sorry to be such a pain on this PR, but this module is used a lot in other projects and I don't want to break other projects builds.

Except for the inline comment I left about the changed configuration, this looks fine to me. If you make this last change, I'm happy to merge and cut a release. I also ran a quick check with these changes and everything looked fine 👍

valotas commented 7 years ago

I removed the code you were referring to at your inline comment. What you must have in mind though is that now I am using poolingInterval for the async pooling too. Previous the timeout used for pooling logs asynchronous was 0 which meant that the driver could get hammered with requests. I had to change the default value of poolingInterval to 100. That makes a new version not backwards compatible.

mantoni commented 6 years ago

This PR is doing too many things and cannot be merged any more. If you want parts of this merged, please open a new PR that ideally focuses on a single change. I feel like parts of this could have been merged right away but where blocked by other changes that needed further discussion. Thank you.