Extend load-config to also work on resources

[imrekoszo]

This could be convenient when kaocha needs to be invoked from code where the contents of the classpath are more reliable than dealing with file system paths.

[plexus]

Thanks, I can imagine this will be useful. Do we have to have a separate function though? What if load-config was polymorphic on its first argument (could be a protocol or multimethod, or just a few instance? checks). Should support String, File, and URL (i.e. "resource").

[imrekoszo]

What if load-config was polymorphic on its first argument (could be a protocol or multimethod, or just a few instance? checks). Should support String, File, and URL (i.e. "resource").

Great point, I'll make that change. Do you mind force pushes to PRs?

[codecov]

Codecov Report

Merging #286 (aa697e4) into main (a505b88) will increase coverage by 0.22%.
The diff coverage is 95.65%.

❗ Current head aa697e4 differs from pull request most recent head b61d163. Consider uploading reports for the commit b61d163 to get more accurate results

@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
+ Coverage   75.37%   75.59%   +0.22%     
==========================================
  Files          51       51              
  Lines        2684     2700      +16     
  Branches      248      248              
==========================================
+ Hits         2023     2041      +18     
+ Misses        504      502       -2     
  Partials      157      157              

Flag	Coverage Δ
integration	57.22% <26.08%> (-0.12%)	⬇️
unit	69.40% <95.65%> (+0.25%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/kaocha/config.clj	90.13% <95.65%> (+2.63%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a505b88...b61d163. Read the comment docs.

[alysbrooks]

Great point, I'll make that change. Do you mind force pushes to PRs?

Force pushes to feature branches are totally OK! We typically ask people to rebase onto main in order to solve merge conflicts.

[imrekoszo]

@alysbrooks @plexus any idea when this could get merged? (if ever)

[plexus]

Hey, thanks for the reminder, I had missed that there was new code here. This looks pretty good! I have one question and one nitpick, both about this section:

  URL ;; resource
  (read-config [resource opts]
   (when resource
     (->> #(merge {;; Ensure we lock #include down to resources
                   :resolver aero/resource-resolver}
                  %)
          (update opts :aero/read-config-opts)
          (read-config-source resource)))))

If I understand your comments correctly Aero by default will look for either files or resources, and here you're saying that if the tests.edn is loaded from a resource, it should only consider resources when looking for #included files. Is that right? It doesn't seem unreasonable but I'm curious about your reasoning. E.g. I can imagine a use case where a base config is coming from a jar, but where people would want a per-project local override (via a #merge [... #include ...]) that's coming from the file system. Is that far fetched? I guess they could standardize on a classpath location (e.g. resources/my_org/tests.local.edn).

My nitpick is having an anonymous function as the first element in a thread-last macro. Normally I would expect a collection there or something that returns a collection. Semantically a thread-first on resource would make more sense, but then there's not much to thread.

Maybe this would be more clear?

(let [opts (update opts :aero/read-config-opts
                   #(merge {:resolver aero/resource-resolver} %))]
  (read-config-source resource opts))

In any case this is not a blocker, I'm fine with merging this as is, but I'm a bit wary of using ->/->> just to flatten any random code, rather than threading what is conceptually a single value across multiple updates.

[imrekoszo]

My nitpick is having an anonymous function as the first element in a thread-last macro

I don't feel strongly either way about it. Let me rewrite the way you suggested so it's more in line with what's normally expected within the project.

If I understand your comments correctly Aero by default will look for either files or resources, and here you're saying that if the tests.edn is loaded from a resource, it should only consider resources when looking for #included files. Is that right? It doesn't seem unreasonable but I'm curious about your reasoning. E.g. I can imagine a use case where a base config is coming from a jar, but where people would want a per-project local override (via a #merge [... #include ...]) that's coming from the file system. Is that far fetched? I guess they could standardise on a classpath location (e.g. resources/my_org/tests.local.edn).

I was somewhat surprised to find that in the currently released version #include "my-overrides.edn" supports resources as well as files. I'm not entirely sure whether this was a conscious decision or just something Aero brought in at some point. By default it uses the adaptive-resolver which first checks for a resource: https://github.com/juxt/aero/blob/743e9bc495425b4a4a7c780f5e4b09f6680b4e7a/src/aero/core.cljc#L129-L134

I find this surprising as out of the box Kaocha advertises it's working off files relative to the directory it's invoked from. In case some dependency includes a resource at "my-overrides.edn" I might be puzzled why my config in the file system is not being loaded.

I didn't want to change this behaviour though as others might already be depending on it.

My aim with locking the other path (where we start with a resource) down more was to be less surprising. Due to how Aero's adaptive-resolver works (resource first, then file), a file-based override could only be used if "my-overrides.edn" does not exist on the classpath anyway.

In the case of the polylith/kaocha integration I'm working on right now, the source of truth is a classpath and I want to make no assumptions as to what is the effective working directory therefore I want to only work with resources. Using adaptive-resolver in this case is I suppose less surprising than in the start-from-file case due to it preferring resources. I still think it's less surprising to know you'll only be dealing with resources so you won't for example forget to make the included file a resource as opposed to just putting it in a file system location from where it loads as a file.

I'm happy to change this to allow for that though, what do you think?

Edit: I just realized as I was refactoring that as of this PR both these default behaviours can be overridden by supplying the correct config in opts. I'm adding notes about that.

[plexus]

Yeah that's entirely reasonable. I don't think there's been a lot of conscious reasoning about the current behavior. The main use cases we've had in mind originally were to have a tests.edn with your project, but to allow alternative tests.edn files to be specified explicitly. We brought in Aero since it provides a great affordances for this type of config file, and with that inherited Aero's #include. I didn't even know that could handle resources as well as files.

Since as you point out an API user can specify explicitly the behavior they want locking it down to be less surprising is I think fine, but a bit of documentation about this would be great, perhaps starting with a docstring on load-config that iterates the things you can pass to it, and how the behaviors are different depending on what you pass it. Going forward it would be good to add a "using Kaocha as an API" section to the docs, I don't think we have that, but that's obviously not your concern now.

[imrekoszo]

docstring on load-config

I'll write something up!

[imrekoszo]

@plexus I refactored that threading macro and added docstring

[plexus]

Thank you so much @imrekoszo for willing to go through a few iterations to make this better! I really appreciate the elaborate docstring.

BTW on Java 8 we're getting an error on CI, in kaocha.watch-test/watch-test

(not (str/includes?
      "[(F)]\n\nFAIL in foo20591.bar-test/xxx-test (bar_test.clj:1)\nExpected:\n  :xxx\nActual:\n  -:xxx +:yyy\n1 tests, 1 assertions, 1 failures.\n\n[watch] Re-running failed tests #{:foo20591.bar-test/xxx-test}\n[(F)]\n\nFAIL in foo20591.bar-test/xxx-test (bar_test.clj:1)\nExpected:\n  :xxx\nActual:\n  -:xxx +:yyy\n1 tests, 1 assertions, 1 failures.\n\n"
      "[(F)]\n\nFAIL in foo20591.bar-test/xxx-test (bar_test.clj:1)\nExpected:\n  :xxx\nActual:\n  -:xxx +:yyy\n1 tests, 1 assertions, 1 failures.\n\n[watch] Reloading #{foo20591.bar-test}\n[watch] Re-running failed tests #{:foo20591.bar-test/xxx-test}\n[(F)]\n\nFAIL in foo20591.bar-test/xxx-test (bar_test.clj:1)\nExpected:\n  :xxx\nActual:\n  -:xxx +:zzz"))

Seems it expects to get a Reloading #{foo20591.bar-test} in the output, but it's not there.

Clearly not related to this PR so I'm going to merge this, but something to keep an eye on. Would be more than nice to not have a flaky CI. This might be related to the change in watchers we did recently? (hawk vs observer)

[plexus]

Released in

[lambdaisland/kaocha "1.66.1034"]
{lambdaisland/kaocha {:mvn/version "1.66.1034"}}

[imrekoszo]

@plexus thank you for the merge!