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

Consider becoming babashka compatible #380

Closed
lread opened this issue May 9, 2022 · 24 comments · Fixed by #440
Closed

Consider becoming babashka compatible #380

lread opened this issue May 9, 2022 · 24 comments · Fixed by #440

Comments

@lread
Copy link
Collaborator

lread commented May 9, 2022

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...

  • it only exposes a subset of the Etaoin APIs
  • needs to be kept in synch with Etaoin
  • means more moving parts

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:

  • http-client with http-client-lite
  • maybe bringing in babashka.fs to replace usage of org.apache.commons.io.FileUtils
  • tweaking babashka to deal with things it currently does not include, if we absolutely need to. For example, I see the usage of java.lang.IllegalThreadStateException which babashka might not currently include.

Additional References

  • Work has already been done by @borkdude within the scope of the babashka Etaoin pod work, he has a graalvm friendly fork of Etaoin. This fork uses http-client-lite, for example.
  • For validation of the idea we have the babashka compatible clj-chrome-devtools, a tool that is probably technically similar to Etaoin.

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
Copy link
Contributor

borkdude commented May 9, 2022

The alternatives:

  • Introduce reader conditionals for babashka differences
  • Maintain a babashka-compatible fork of this repo

@igrishaev
Copy link
Collaborator

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
Copy link
Contributor

@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
Copy link
Collaborator Author

lread commented May 10, 2022

@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
Copy link
Collaborator Author

lread commented May 10, 2022

Looking at the comparison against @borkdude's fork 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
Copy link
Collaborator

sure @lread please go ahead!

@igrishaev
Copy link
Collaborator

@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
Copy link
Collaborator Author

lread commented May 11, 2022

@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
Copy link
Contributor

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
Copy link
Collaborator

@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
Copy link
Member

slipset commented May 11, 2022

@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
Copy link
Collaborator Author

lread commented May 20, 2022

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:

(defmacro with-pool [opt & body]
`(client/with-connection-pool ~opt
~@body))

Which it also uses internally:

etaoin/src/etaoin/api.clj

Lines 3439 to 3467 in 289a591

(defmacro with-driver
"Performs the body within a driver session.
Launches a driver of a given type. Binds the driver instance to a
passed `bind` symbol. Executes the body once the driver has
started. Shutdowns the driver finally (even if an exception
occurred).
Arguments:
- `type` is a keyword what driver type to start.
- `opt` is a map with driver's options. See `boot-driver` for more
details.
- `bind` is a symbol to bind a driver reference.
Example:
(with-driver :firefox {} driver
(go driver \"http://example.com\"))
"
[type opt bind & body]
`(client/with-pool {}
(let [~bind (boot-driver ~type ~opt)]
(try
~@body
(finally
(quit ~bind))))))

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
Copy link
Contributor

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
Copy link
Contributor

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

@borkdude
Copy link
Contributor

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
Copy link
Collaborator Author

lread commented May 21, 2022

I wonder if connection pooling is important to Etaoin users too.
There are use cases of hitting remote webdrivers, if I understand correctly: #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
Copy link
Contributor

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
Copy link
Collaborator Author

lread commented May 21, 2022

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
Copy link
Contributor

Proceed.

lread added a commit to lread/etaoin that referenced this issue May 21, 2022
Etaoin was using apache commons.io to delete a tempoarary dir.
This dependency was brought in implicitly by clj-http.

Replaced with babashka/fs delete-tree.

Out of scope: Using more babashka/fs than we need to. This libary
has some great abstractions that I expect will help with maintainability,
we'll get to this later.

Contributes to clj-commons#380
@lread
Copy link
Collaborator Author

lread commented May 21, 2022

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 added a commit that referenced this issue May 21, 2022
road to bb: tweak process support

Proc was referencing java.lang.IllegalThreadStateException, an exception
that babashka is currently unaware of.

Good news is that the code that referenced this is entirely unused.
Deleted it along with other unused vars.

Also: marked private vars as such
And: tweaked proc, proc-test to help maybe diagnose flakiness on Windows on CI
- Add a .waitFor after killing process.
- Move proc-tests out from under `testing` to their own `deftest`s.
- Have each test use its own port.
- And dump processes that are running in addition to chromedriver processes
found. We'll remove this last bit after we achieve stability.

Contributes to #380
@lread
Copy link
Collaborator Author

lread commented May 22, 2022

@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
Copy link
Contributor

@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
Copy link
Collaborator Author

lread commented May 22, 2022

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
Copy link
Contributor

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

lread added a commit to lread/etaoin that referenced this issue May 22, 2022
Apologies to any lein lovers out there!

Reference to lein either converted to clojure cli or deleted.

- Figuring out how to call the official clojure distribution on Windows
is an exercise in frustration. Instead we use babashka's clojure function to
launch clojure. Works like a charm.
- Added new bb task to download clojure deps for CI
- Added an anemic pom.xml to preserve data previously held by project.clj.
This will be used by release flow once I get to clj-commons#432.
- Unlike lein, the clojure test-runner does not allow selection of tests
based on metadata at the namespace level. To easily distinguish unit tests I
moved them under a `unit` directory.
- I noticed that ide tests were failing for me.
This is because we are now testing with our documented minimum Clojure
version of 1.9. The ide code was taking advantage of a 1.10 feature.
Making it 1.9 compatible was an easy fix.
- Switched to logback for logging for dev and testing to avoid any log4j
security concern stink.
- Noticed that we had `./resources/` `./env/dev/resources` and
`./env/test/resources`. But `resources/` contains only test resources, so I
moved it under `./env/test/resources`.  `./env/dev/resources` seems to
be a way to turn on debug logging, so left it as that and created a
deps.edn `:debug` alias for it.
- Our Makefile is getting smaller. Docker tasks now certainly broken.
I'll make a separate issue to deal with this (and move to bb tasks).

Helps me with clj-commons#380
Closes clj-commons#432
lread added a commit to lread/etaoin that referenced this issue May 22, 2022
Apologies to any lein lovers out there!

Reference to lein either converted to clojure cli or deleted.

- Figuring out how to call the official clojure distribution on Windows
is an exercise in frustration. Instead we use babashka's clojure function to
launch clojure. Works like a charm.
- Added new bb task to download clojure deps for CI
- Added an anemic pom.xml to preserve data previously held by project.clj.
This will be used by release flow once I get to clj-commons#422.
- Unlike lein, the clojure test-runner does not allow selection of tests
based on metadata at the namespace level. To easily distinguish unit tests I
moved them under a `unit` directory.
- I noticed that ide tests were failing for me.
This is because we are now testing with our documented minimum Clojure
version of 1.9. The ide code was taking advantage of a 1.10 feature.
Making it 1.9 compatible was an easy fix.
- Switched to logback for logging for dev and testing to avoid any log4j
security concern stink.
- Noticed that we had `./resources/` `./env/dev/resources` and
`./env/test/resources`. But `resources/` contains only test resources, so I
moved it under `./env/test/resources`.  `./env/dev/resources` seems to
be a way to turn on debug logging, so left it as that and created a
deps.edn `:debug` alias for it.
- Our Makefile is getting smaller. Docker tasks now certainly broken.
I'll make a separate issue to deal with this (and move to bb tasks).

Helps me with clj-commons#380
Closes clj-commons#432
lread added a commit to lread/etaoin that referenced this issue May 22, 2022
We were using java's ImageIO to validate that we could successfully read
screenshot png images. There's nothing wrong with this, but babashka
does not currently include ImageIO support.

So I've switch to calling out to Image Magick to validate our screenshot
png are valid.

A minor developer setup burden for bb compatibility.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 22, 2022
We were using java's ImageIO to validate that we could successfully read
screenshot png images. There's nothing wrong with this, but babashka
does not currently include ImageIO support.

So I've switch to calling out to Image Magick to validate our screenshot
png are valid.

A minor developer setup burden for bb compatibility.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 22, 2022
We were using java's ImageIO to validate that we could successfully read
screenshot png images. There's nothing wrong with this, but babashka
does not currently include ImageIO support.

So I've switch to calling out to Image Magick to validate our screenshot
png are valid.

A minor developer setup burden for bb compatibility.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 22, 2022
We were using java's ImageIO to validate that we could successfully read
screenshot png images. There's nothing wrong with this, but babashka
does not currently include ImageIO support.

So I've switch to calling out to Image Magick to validate our screenshot
png are valid.

A minor developer setup burden for bb compatibility.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 22, 2022
We were using java's ImageIO to validate that we could successfully read
screenshot png images. There's nothing wrong with this, but babashka
does not currently include ImageIO support.

So I've switch to calling out to Image Magick to validate our screenshot
png are valid.

A minor developer setup burden for bb compatibility.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 22, 2022
We were using java's ImageIO to validate that we could successfully read
screenshot png images. There's nothing wrong with this, but babashka
does not currently include ImageIO support.

So I've switch to calling out to Image Magick to validate our screenshot
png are valid.

A minor developer setup burden for bb compatibility.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 22, 2022
We were using java's ImageIO to validate that we could successfully read
screenshot png images. There's nothing wrong with this, but babashka
does not currently include ImageIO support.

So I've switch to calling out to Image Magick to validate our screenshot
png are valid.

A minor developer setup burden for bb compatibility.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 22, 2022
We were using java's ImageIO to validate that we could successfully read
screenshot png images. There's nothing wrong with this, but babashka
does not currently include ImageIO support.

So I've switch to calling out to Image Magick to validate our screenshot
png are valid.

A minor developer setup burden for bb compatibility.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 22, 2022
We were using java's ImageIO to validate that we could successfully read
screenshot png images. There's nothing wrong with this, but babashka
does not currently include ImageIO support.

So I've switch to calling out to Image Magick to validate our screenshot
png are valid.

A minor developer setup burden for bb compatibility.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 22, 2022
We were using java's ImageIO to validate that we could successfully read
screenshot png images. There's nothing wrong with this, but babashka
does not currently include ImageIO support.

So I've switch to calling out to Image Magick to validate our screenshot
png are valid.

A minor developer setup burden for bb compatibility.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 22, 2022
We were using java's ImageIO to validate that we could successfully read
screenshot png images. There's nothing wrong with this, but babashka
does not currently include ImageIO support.

So I've switch to calling out to Image Magick to validate our screenshot
png are valid.

A minor developer setup burden for bb compatibility.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 22, 2022
We were using java's ImageIO to validate that we could successfully read
screenshot png images. There's nothing wrong with this, but babashka
does not currently include ImageIO support.

So I've switch to calling out to Image Magick to validate our screenshot
png are valid.

A minor developer setup burden for bb compatibility.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 23, 2022
We were using java's ImageIO to validate that we could successfully read
screenshot png images. There's nothing wrong with this, but babashka
does not currently include ImageIO support.

So I've switch to calling out to Image Magick to validate our screenshot
png are valid.

A minor developer setup burden for bb compatibility.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 23, 2022
Replaced direct JDK references with babashka fs abstractions.

Of note:
- turfed etaoin.util/with-tmp-dir and replaced its usages with babashka
fs equivalent (Files/createTempDirectory is available in bb, I think).
We've decided that etaoin.util is not part of our public API clj-commons#430, so not
worried about deleting this macro.
- some references to io/file replaced with fs/file for more flexible coercion.

Contributes to clj-commons#380
lread added a commit that referenced this issue May 23, 2022
Replaced direct JDK references with babashka fs abstractions.

Of note:
- turfed etaoin.util/with-tmp-dir and replaced its usages with babashka
fs equivalent (Files/createTempDirectory is unavailable in bb, I think).
We've decided that etaoin.util is not part of our public API #430, so not
worried about deleting this macro.
- some references to io/file replaced with fs/file for more flexible coercion.

Contributes to #380
lread added a commit to lread/etaoin that referenced this issue May 23, 2022
When running babashka, we now use the compatible http-client-lite.
(Thanks borkdude your etaoin pod work was a very useful reference!)

Reader conditionals select the existing http-client when running on from
the JVM.

Reader conditionals mean .cljc, which often means Clojure and
ClojureScript, but in our case it means JVM Clojure and Babashka
Clojure. Update configs for cljdoc and clj-kondo to let them know
that this is not a ClojureScript project.

Facility to test our bb implementation added via a new --platform option
on our `bb test` task. I'm not sure of the best way to run babashka
tests, but my first stab generates a runner.clj with appropriate
namespaces and classpath then runs that.

Etaoin does have some debug logging, so Babashka's log level is adjusted
from the default of debug to info in the generated test runner.

Finally adjusted our CI matrix to include the bb platform.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 23, 2022
When running babashka, we now use the compatible http-client-lite.
(Thanks borkdude your etaoin pod work was a very useful reference!)

Reader conditionals select the existing http-client when running on from
the JVM.

Reader conditionals mean .cljc, which often means Clojure and
ClojureScript, but in our case it means JVM Clojure and Babashka
Clojure. Update configs for cljdoc and clj-kondo to let them know
that this is not a ClojureScript project.

Facility to test our bb implementation added via a new --platform option
on our `bb test` task. I'm not sure of the best way to run babashka
tests, but my first stab generates a runner.clj with appropriate
namespaces and classpath then runs that.

Etaoin does have some debug logging, so Babashka's log level is adjusted
from the default of debug to info in the generated test runner.

Finally adjusted our CI matrix to include the bb platform.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 23, 2022
When running babashka, we now use the compatible http-client-lite.
(Thanks borkdude your etaoin pod work was a very useful reference!)
(I worked around an issue in http-client-lite when running on Windows.
See clj-commons/clj-http-lite#18)

Reader conditionals select the existing http-client when running on from
the JVM.

Reader conditionals mean .cljc, which often means Clojure and
ClojureScript, but in our case it means JVM Clojure and Babashka
Clojure. Update configs for cljdoc and clj-kondo to let them know
that this is not a ClojureScript project.

Facility to test our bb implementation added via a new --platform option
on our `bb test` task. I'm not sure of the best way to run babashka
tests, but my first stab generates a runner.clj with appropriate
namespaces and classpath then runs that.

Etaoin does have some debug logging, so Babashka's log level is adjusted
from the default of debug to info in the generated test runner.

Finally adjusted our CI matrix to include the bb platform.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 23, 2022
When running babashka, we now use the compatible http-client-lite.
- Thanks borkdude, your etaoin pod work was a very useful reference!
- I worked around an issue in http-client-lite when running on Windows.
See clj-commons/clj-http-lite#18.

Reader conditionals select the existing http-client when running on from
the JVM.

Reader conditionals mean .cljc, which often means Clojure and
ClojureScript, but in our case it means JVM Clojure and Babashka
Clojure. Update configs for cljdoc and clj-kondo to let them know
that this is not a ClojureScript project.

Facility to test our bb implementation added via a new --platform option
on our `bb test` task. I'm not sure of the best way to run babashka
tests, but my first stab generates a runner.clj with appropriate
namespaces and classpath then runs that.

Etaoin does have some debug logging, so Babashka's log level is adjusted
from the default of debug to info in the generated test runner.

Finally adjusted our CI matrix to include the bb platform.

Contributes to clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 24, 2022
lread added a commit to lread/etaoin that referenced this issue May 24, 2022
Reviewed and updated start of user guide and ensured first getting
started examples still run today.

Will do more verification of rest of doc (it is big!) bit by bit in the
future.

Closes clj-commons#380
lread added a commit to lread/etaoin that referenced this issue May 24, 2022
Reviewed and updated start of user guide and ensured first getting
started examples still run today.

Will do more verification of rest of doc (it is big!) bit by bit in the
future.

Closes clj-commons#380
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 a pull request may close this issue.

4 participants