Closed alysbrooks closed 1 year ago
Base: 75.58% // Head: 75.34% // Decreases project coverage by -0.25%
:warning:
Coverage data is based on head (
64f67e8
) compared to base (52f42b7
). Patch coverage: 47.36% of modified lines in pull request are covered.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Looking great @alysbrooks , I riffed off this some more here: https://github.com/lambdaisland/kaocha/pull/387 . Not fully baked yet but we're clearly in downhilll territory now.
cc @borkdude @fdserr
@plexus @alysbrooks Great progress. If you want, I'd be happy contribute a github actions yml file where we execute tests in bb latest
. If you find any strange things that should be solved in bb or have any questions, ping me.
@borkdude If it's something existing you can mostly just drop in that would be helpful. We might want to move it to CircleCI for consistency with the rest of our tests, so I don't want you putting in a ton of work to adapt it.
@alysbrooks My thought is that adding a github actions runner for bb doesn't interfere as much with your existing tooling.
Also check out this comment where we did a similar thing for deep-diff2:
https://github.com/lambdaisland/deep-diff2/pull/36#issuecomment-1327240979
I think moving Arne's refactorings to kaocha.platform.*
makes sense. I think these are inherently a lower layer and if we ever add support for other runtimes (possibly ones that don't even exist yet), this will help organize that work.
Careful about making breaking changes, there might be plugins that rely on kaocha.classpath, I don't think we can move that.
@plexus Is kaocha.classpath intended as a public API? A quick glance on grep.app shows that no public repositories are using it yet: https://grep.app/search?q=kaocha.classpath. Nevertheless, may be good to err on the safe side.
I think it would be valid for a plugin to rely on that API (same for other internal libraries like kaocha.testable or kaocha.load). We generally avoid renaming things for the sole reason that "it's nicer this way". It potentially disrupts the workflow of someone downstream, or causes churn for downstream maintainers, and so it's not worth the risk, even if the chance is quite small.
I agree, as long as it's clear what is public and private API: never change the public API.
Kaocha-cloverage actually does use kaocha.classpath
(and that's actually why the tests are failing). Obviously we can change it, but I think it illustrates Arne's point that other libraries might use it.
I'm struggling to figure out why the test is no longer working it seems like it is both getting stuck and not timing out properly. Maybe we should disable the test for now?
It seems the test still works on the main branch, right? If so, then there must be a chance which makes this test fail... somewhere? :)
Slight tweak to some of our integration tests to prevent them from hanging on failure. Now they still fail, but do so right away, like you'd hope.
As I included in my last commit message, I don't know if there are some situations where the process would still be unspooling information, and blocking on slurp for a brief moment allows for capturing that information. I think missing out on that is acceptable since you are already in a failure state and hanging is much worse. Adding a short delay could mitigate this, however.
I nearly forgot. The change causing the test to fail is kaocha.util/compiler-exception-file-and-line
. It slightly tweaks the output when tests fail, causing that integration test to no longer match. The exact change is that instead of just printing the file name, we now print the whole path.
Kaocha's own test suite is problematic, but it seems to run well on more typical test suites.
I'm merging this, I think we've gone over this thoroughly enough at this point. Let's cut a release and see if anyone runs into issues.
@alysbrooks what's this about? Our matcher-combinators version is pinned to 1.5.x, we never migrated to the newer versions, so this will certainly make our tests fail.
;; deps.edn
{,,,
:aliases
{,,,
:bb
{:extra-deps {nubank/matcher-combinators {:mvn/version "3.5.1"}}}}}
Released in v1.79.1270
[lambdaisland/kaocha "1.79.1270"] ;; deps.edn
{lambdaisland/kaocha {:mvn/version "1.79.1270"}} ;; project.clj
@plexus @alysbrooks Exciting! I think one thing might still be missing: some tests (at least some smoke tests) against bb latest
so it's apparent when something is introduced that breaks bb compatibility. If you're open to a PR, I willing to to this work (later this week) like I did for some other libs recently, if it's ok to run a Github runner. If CircleCI is a must, then I'd rather defer that to your team since I'd have to figure that out more.
A PR for that would be great @borkdude , and using GH actions is fine too. Thank you! I personally dislike Circle and GH actions in equal amounts so no particular preference ;)
I'm also using both, but lambdaisland is doing way more advanced stuff with CircleCI than I have :)
Yeah I once put a bunch of effort into creating these docker images and associated "orbs", so people would have a convenient way of setting up clojure/kaocha on circle ci , but in the end we never really promoted them so I guess it's mostly just us using them. It's also a mess of YAML templating which... does not spark joy.
This requires Babashka 1.1.171 and disabling the
:notifier
plugin (for now).In contrast to my experiment branch
alys/bb-experiment
, this PR doesn't use any conditionals. Instead, we create a newclasspath.bb
and a macroif-babashka
Why not use conditionals?
.cljc
files may communicate that these namespaces run on ClojureScript.Using conditionals does have the advantage of being something tried and true, compared to a new macro.
Remaining steps: