search

clojure-emacs / orchard

A fertile ground for Clojure tooling
Eclipse Public License 1.0
323 stars 54 forks source link

Misc maintenance #257

Closed alexander-yakushev closed 2 months ago

alexander-yakushev commented 2 months ago

Copying question from Slack:

I just noticed that Orchard test matrix separately runs tests with and without the :provided profile, can you please confirm that this still has a purpose and which one?

vemv commented 2 months ago

IIRC the purpose is ensuring that Orchard is able to run in both the presence and absence of the clojurescript dependency.

Some code conditionally obseves the presence of cljs, so by maintaining this matrix, we hit both branches of the code.

alexander-yakushev commented 2 months ago

Given that Orchard CLJS features don't intersect with anything that Java brings and with javadoc parsing, would it make sense to trim the matrix there? Pick a single parser type for cljs tests and the latest JDK (or oldest, or both) for Clojurescript-included tests. This can save us ~20 testing jobs per run.

vemv commented 2 months ago

I'd be OK with shaving parsing stuff, but I'd still run all JDK<->cljs combinations, out of caution.

alexander-yakushev commented 2 months ago

How about doing it the other way around? Test everything with CLJS included, but have a couple sanity-check jobs without it on the classpath. Not having it on classpath only tests that Orchard correctly requires it conditionally, right? Should be enough to do it once or twice.

vemv commented 2 months ago

Depends on the nature of the sanity check. I'd be open to check it out but, please be advised, I tend to lean towards safety.

In practice, the CI matrix hasn't been a bottleneck since its introduction some 3y ago.

Normally if one job fails/passes, that's all the info I need and I can focus elsewhere till it's all complete (which is parallel).

(plus, one has to wait for a code review anyway)

In return we get a fair bit of peace of mind - sometimes we get pretty odd issue reports, so ruling out some basics helps in quickly diagnosing user reports as they happen.

alexander-yakushev commented 2 months ago

Circle begins to throttle parallelism when pushing code actively, so it gets mildly annoying. Safety is nice, but it also doesn't make sense to waste cycles just for the sake of it. GIven the multiplicative nature of CI matrix dimensions, there has to be a limit where the spread has to stop.

Do you have off top of your head an example of a report that was caused by a weird combination of parameters? That would certainly change my mind.

alexander-yakushev commented 2 months ago

Last commit is an example how it can look like. Brings it to 55->34 jobs.

alexander-yakushev commented 2 months ago

Another point that I have previously brought up is Eastwood on every matrix cell. Last time you've mentioned that because it loads code, then running it CLJxJDK makes sense, and I agree. But doing that for every parser combination is wasteful (since it lints namespaces all the same).

bbatsov commented 2 months ago

Agreed. The current setup seems excessive and we can optimize it.

On Tue, Apr 30, 2024, at 5:49 PM, Oleksandr Yakushev wrote:

Another point that I have previously brought up is Eastwood on every matrix cell. Last time you've mentioned that because it loads code, then running it CLJxJDK makes sense, and I agree. But doing that for every parser combination is wasteful (since it lints namespaces all the same).

— Reply to this email directly, view it on GitHub https://github.com/clojure-emacs/orchard/pull/257#issuecomment-2085732596, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZLSRHMZ2XKJ6GHZXSNK3Y764PZAVCNFSM6AAAAABHALSZL6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBVG4ZTENJZGY. You are receiving this because you are subscribed to this thread.Message ID: @.***>

vemv commented 2 months ago

But doing that for every parser combination is wasteful (since it lints namespaces all the same).

👍 good to slash that part

Circle begins to throttle parallelism when pushing code actively, so it gets mildly annoying. Safety is nice, but it also doesn't make sense to waste cycles just for the sake of it. GIven the multiplicative nature of CI matrix dimensions, there has to be a limit where the spread has to stop.

I'd just really appreciate exercising the JDK*cljs part. It helps me as maintainer to rule out that possible source of issues.

The matrix has helped us out in many forms. Asking for a specific example is too much of an ask (when any given day there many better uses of my time), suffice to say that very few people actively maintain the cljs part, so let's try to find middle grounds.

The build is pretty quick and if you commit N times in a row, it's easy enough to go over CCI and stop older commits.

In general, let's try to balance out dev convenience vs. user-facing correctness. Neither extreme is the best choice.

Cheers - V