search

clj-commons / etaoin

Pure Clojure Webdriver protocol implementation
https://cljdoc.org/d/etaoin
Eclipse Public License 1.0
921 stars 96 forks source link

Consider becoming babashka compatible #380

Closed lread closed 2 years ago

lread commented 2 years ago

Currently

Etaoin is an awesome library that is runnable from Clojure. It is also runnable from babashka via pod-babashka-etaoin.

The babashka Etaoin pod is very handy but...

What if...

We could run Etaoin directly from babashka without a pod?

Does this make sense?

Initial look

At its technical heart, Etaoin seems to be mostly a REST client.

For babashka compatibility we might look at replacing:

Additional References

Next Steps

If you agree that this is interesting, I'd be happy to follow up with a deeper investigation, and if all is viable, a PR.

borkdude commented 2 years ago

The alternatives:

igrishaev commented 2 years ago

I agree it would be great to make it bb-frienldy. Recently I thought about replacing clj-http with clj-http-lite. Honeslty, I haven't touched Etaoin for months so I need to check first what should be done. Any help, comments, suggestions is welcome.

borkdude commented 2 years ago

@igrishaev Great! My fork https://github.com/borkdude/etaoin-graal already uses clj-http-lite and is only slightly behind this upstream repo. Another option could be to use the JDK 11 http client, which is great, but ... requires JDK 11 :)

lread commented 2 years ago

@igrishaev I am happy to attempt a PR if that would be of help. But I also totally understand if you'd rather do the work yourself. Lemme know!

lread commented 2 years ago

Looking at the comparison against @borkdude's fork https://github.com/igrishaev/etaoin/compare/master...borkdude:master: I guess we'd have to make sure we aren't relying on any http-client feature, but switching to http-client-lite seems otherwise straightforward.

igrishaev commented 2 years ago

sure @lread please go ahead!

igrishaev commented 2 years ago

@lread @borkdude may I ask something: can one of you (or you both) become official maintainers of Etaoin? The thing is, I'm completely far away from the UI and browser testing stuff. I've been working actively on it several years ago being involved in a UI-driven project. But for the last 3 years I have only backend. I feel guilty because people come and propose something useful but I'm blocking them. Wdyt? If not, I can make an announcement in Slack or in Clojure Planet.

lread commented 2 years ago

@igrishaev would you still be involved/available? Would you still want to be involved in decisions/direction?

I'm not an expert in this area, but I am happy to help out with maintaining Etaoin. Warning: if you give me too much power, I WILL introduce clj-kondo. 🙂

borkdude commented 2 years ago

Perhaps we should transfer it to clj-commons then and maintain it in that organization? I'm ok with being involved for general advice and opinions, although I'm also not an expert :).

igrishaev commented 2 years ago

@lread

are would you still be involved/available?

well, not much. I want this project live by its own. I can participate in some discussions if needed, though.

@borkdude

Perhaps we should transfer it to clj-commons then and maintain it in that organization?

yes I like the idea. What would be our steps?

slipset commented 2 years ago

@igrishaev I've invited you as a member to cli-commons. Once you've accepted that membership, you can transfer this repo to cli-commons. LMK if you have any problems

lread commented 2 years ago

Ok, after working on a few other issues, I think I am now finally ready to get to the real fun and start working on this issue.

One thing I noticed: Etaoin exposes clj-http's with-connection-pool macro: https://github.com/clj-commons/etaoin/blob/289a591a0a29b2b8312f316020de6f371314d598/src/etaoin/client.clj#L48-L50 Which it also uses internally: https://github.com/clj-commons/etaoin/blob/289a591a0a29b2b8312f316020de6f371314d598/src/etaoin/api.clj#L3439-L3467

But... this is something that clj-http-lite currently opts out of: https://github.com/clj-commons/clj-http-lite/blob/6b53000df55ac05c4ff8e5047a5323fc08a52e8b/src/clj_http/lite/client.clj#L295-L300

borkdude commented 2 years ago

Would it be an idea to use the JDK 11 client for the bb side of things? It has a default connection pool:

https://stackoverflow.com/a/53620696/6264

borkdude commented 2 years ago

An easy thin layer over the JDK 11 client is https://github.com/schmee/java-http-clj which also works in bb.

borkdude commented 2 years ago

Thinking more about it: connection pooling may not be at all so important for this library as the chromedriver usually runs on a local computer, so connecting to it should be really fast. Actually driving the browser probably takes way more time than the socket overhead. So maybe it's just fine to leave this as it is in the babashka impl with clj-http-lite.

lread commented 2 years ago

I wonder if connection pooling is important to Etaoin users too. There are use cases of hitting remote webdrivers, if I understand correctly: https://github.com/clj-commons/etaoin/pull/357.

Moving to a minimum of JDK11 seems reasonable, and will only become more reasonable with each passing day. For those still stuck on JDK8, for whatever reason, there is still current version of Etaoin. (anyone reading this who absolutely needs JDK8 please do chime in).

In the spirit of brainstorming. Some more (not necessarily good) ideas (if pooling and JDK8 are actually important for JVM use but not Babashka use):

  1. Separate http support to etaoin http artifact. You use etaoin by including etaoin and either etaoin-http or etaoin-http-lite dependencies
  2. Always publish 2 jars: etaoin and etaoin-lite. The etaoin-lite jar would be used by bb.
borkdude commented 2 years ago

I think there is a potential interesting 3rd option

We could just use reader conditionals (or macros) to make bb use JDK 11 (via java-http-clj for example) or clj-http-lite.

lread commented 2 years ago

We could just use reader conditionals (or macros) to make bb use JDK 11 (via java-http-clj for example) or clj-http-lite.

Oh right! We'd still have the clj-http dependency, but because under bb Etaoin code would never use it, no problemo.

I like this idea the most. Agreed that I proceed with it? Or do we need to think more?

borkdude commented 2 years ago

Proceed.

lread commented 2 years ago

After being so proud of myself for getting all tests running on all platforms (in preparation for bb compatibility work), it just occurred to me that I should get an idea of what is not covered by current Etaoin tests.

I'll try to run a cloverage report locally sometime soon and report back.

lread commented 2 years ago

@borkdude, nothing comprehensive yet, I've only tried macOS with chrome, but for this scenario all tests are passing when run under bb. 🎉 I will push something after some cleanup.

There was a use of ImageIO in tests to verify a generated screenshot png was readable, but I decided to replace this check with a call out to Image Magick's identify instead.

One thing that I know I need your advice on is Clojure spec. I found your babashka/spec.alpha, and when included, it seems to work just fine with Etaoin's usage of spec. 🥳

But two questions:

  1. will including this lib interfere with non-bb (JVM) usage at all?
  2. is this lib a candidate for clojars? I can test with a git dep, but think I'll need an mvn dep before releasing.

Time for sleep!

borkdude commented 2 years ago

@lread Yes, it will interfere, so I suggest not including it in etaoin, but bb users should include it in their bb.edn setup.

Are specs really essential to etaoin? I often recommend making a separate specs.clj namespace so they can be optionally loaded if people want to instrument their functions. Maybe we can do that here too?

lread commented 2 years ago

Thanks @borkdude, that helps me to understand!

Specs are currently integral to one Etaoin feature: running Selenium IDE files. It seems reasonable to want to always validate commands in these .ide files, and Etaoin currently uses spec to do so.

While running Selenium IDE files is a very cool feature, I don't think it is the primary use of Etaoin. So I'm comfortable with documenting that babashka/specs-alpha needs to be included as a dependency when folks want to use this feature.

If we do learn that this is too awkward for bb Etaoin users, we can always adapt, but I think this would be a good first strategy.

borkdude commented 2 years ago

Well, that's how babashka users should already deal with spec, so not a problem.