Support scanning tests in src directories

[tengstrand]

People sometimes put test code together with the src code. Today we don't run those tests. It would be good if this could be configurable.

To turn it on globally, we could put an :include-src-dirs flag in workspace.edn at the root to affect all tests:

{
 ...
 :test {:include-src-dirs true}
}

or per project:

{
 ...
 :projects {"myproject" {...
                         :test {:include-src-dirs true}}
}

or even per brick:

{
 ...
 :bricks {"mybrick" {...
                     :test {:include-src-dirs true}}
}

[marksto]

Totally support this feature of poly test, but I must admit that I never run into the problem myself, neither with rcf nor with rich-comment-tests on a Polylith-based project that used both.

[seancorfield]

I use rich-comment-tests (per marksto above) but we follow the recommendation in RCT to have a stub file in the test tree that calls into the relevant source ns to actually run the inline tests as part of regular testing.

I would never want src folders scanned for tests but this seems like a harmless (if somewhat pointless) enhancement as long as it is OFF by default.

Plenty of tooling out there doesn't support tests in src and I call it out as a problematic approach in the Expectations documentation because of that.

[tengstrand]

This issue was created to make it easier to work with Hyperfiddle rcf as described here. As it is today, you can add the src directory to the test paths to achieve this, but making it configurable would make it easier for these users. I can't estimate how important this is and if it's worth supporting. Sean points out that it's a problematic approach. What do you say @ieugen?

[ieugen]

hi @tengstrand , I am a bit unsure after reading the comments.

I will try out https://github.com/matthewdowney/rich-comment-tests and probably migrate to it - since it seems a bit less intrusive than RCF .

In gradle, src classes are available for tests on the classpath.
Not sure why with clojure is (seems) different.

If the feature is easy to implement and maintain than I am ok with the approach.
As an alternative to a feature that would need to be maintained, this could be fixed via improving documentation maybe?

My experience with Clojure is a bit more limited than some.
I do have experience from Java land and gradle specifically and I remember this diagram https://docs.gradle.org/current/userguide/java_library_plugin.html#sec:java_library_configurations_graph .

In gradle, testClasspath contains/has access to compileClasspath .
I guess for clojure this is a bit different since clojure has the reader ?!?

[seancorfield]

FWIW, the https://github.com/seancorfield/polylith-external-test-runner supports :include-src-dir true (as well as :focus which lets you either specify a list of specific test :var fully-qualified symbols to run or metadata-based :include / :exclude -- mimicking Cognitect's test-runner options).

Polylith 0.2.20 (SNAPSHOT) supports a :test-configs map in workspace.edn that lets you provide named sets of test runner options,

[tengstrand]

Can we close this issue?

[seancorfield]

As long as you're happy with solving this in specific test runners through their specific config, sure. I don't care whether the built-in test runner supports this :)

[seancorfield]

Seems like this could be closed out now.

[imrekoszo]

I remembered a (n edge) case about this that I encountered when working on polylith-kaocha: if a brick does not have test paths, then the poly tool itself won't consider that brick for testing in (:bricks-to-test project) even if there are tests in its src directory (like rcf, test metadata, kaocha's auto-generated ones from s/fdefs). To support this case I had to hack a bit in polylith-kaocha so that works there now. It doesn't seem to work with the external test runner atm.

I created a repro case at https://github.com/imrekoszo/poly-test-in-src

[seancorfield]

Interesting edge case. I guess another workaround is to have a dummy test ns in such bricks?

I think I'd be inclined to just document this edge case, and encourage users to add at least a dummy test ns to get poly test to include the brick?

The default behavior for most (all?) test runners is not to look for tests in sources, and I get the impression it is discouraged (since you often don't want test code in your deployable artifacts). For the RichCommentTests library (not rcf), the recommendation is to create an explicit test file that invokes tests from RCFs in the source code -- which is an approach we use at work.

[imrekoszo]

Thanks Sean. I added the dummy test workaround to the repro repo.

I noticed two differences between the two workarounds:

1. dummy test workaround still needs :include-src-dir set
2. dummy test workaround doesn't make the built-in runner discover the src tests (unless it's combined with the other workaround)

If documented well, both these should enable the removal of the hack from polylith-kaocha. I kinda prefer the duplicate paths workaround for the following reasons:

• it needs one less special case in test runners (no need to support include-src-dir)
• looks like Poly treats extra paths in the :test alias as "the current brick's tests can be found here" - so it sort of makes sense to me to tell Poly that the tests are under src if that's the case.

[seancorfield]

I would expect 1. :include-src-dir is a possible option for test runners to optionally include the :src selector when building brick-test-namespaces and project-test-namespaces.

The built-in test runner doesn't support :include-src-dir so I would expect 2. as well. include-src-dir isn't mentioned anywhere in Polylith's docs (or source) because, currently, it simply passed through the test runner.

I'll repeat my comments from early March '24:

I would never want src folders scanned for tests but this seems like a harmless (if somewhat pointless) enhancement as long as it is OFF by default.

Plenty of tooling out there doesn't support tests in src and I call it out as a problematic approach in the Expectations documentation because of that.

Test runners don't have to support this. Some test runners do, and in their docs, they can explain how they want it to work:

• either explain :include-src-dir and the workaround for the empty test folder case,
• or explain how to update :test > :extra-paths in bricks to make src nses available as "test" namespaces for this case, for the bricks where they want tests in src.

I will update my external test runner's docs for the first case there.

It sounds like you'd prefer the Kaocha runner to follow the second case there (and remove support for :include-src-dir since you'd rather update :test > :extra-paths instead).

[imrekoszo]

Thanks for the brainstorming, Sean! I ended up adding a new property to project in poly so runners that want to support this more seamlessly can use :bricks-to-test-all-sources instead of :bricks-to-test.

[seancorfield]

@imrekoszo Looking over the changes, it seems like the only effect for my external test runner would be to change bricks-to-test to bricks-to-test-all-sources and the only impact of that change is to include bricks that have no tests (that are changed or depend on changed bricks)?

I'd still have the same code to select either test code only or test + source code (if :include-src-dir true), right?

And, to confirm, I'd only need to switch the key I use in project if :include-src-dir true?

[imrekoszo]

@seancorfield that's correct. bricks-to-test-all-sources is a superset of bricks-to-test, including bricks that don't have dedicated test sources but are otherwise selected for testing.

And, to confirm, I'd only need to switch the key I use in project if :include-src-dir true?

That also sounds correct. Although you don't need to, you made pretty strong arguments for not encouraging keeping tests under src. My case is different in that Kaocha has its own handling of this topic and I just want to make the experience smooth for those that want to rely on that.

[seancorfield]

Thanks. I've updated my external test runner and I'm planning an 0.7.0 release as Polylith reaches its 0.2.22 release.