bin/eftest will succeed with 0 tests if it can't load namespaces

[danielcompton]

We ran into a situation where code was pushed in a PR that wouldn't compile. However, bin/eftest counted 0 tests and no compile error was thrown, so it exited successfully with 0 tests.

The direct cause of the problem is that the implementation of orchard.query/namespaces tries to require a namespace, but doesn't throw an error if the require fails.

https://github.com/clojure-emacs/orchard/blob/v0.4.0/src/orchard/namespace.clj#L17-L22

Could we instead use eftest's find-tests directly by passing it "test" for the test directory? It's not fully general, but it seems like it could be the kind of useful default that edge provides.

Zooming out further, this isn't the first time I've hit this class of bug. I've had other cases in the past where no tests can be detected due to a bigger problem, and the test run erroneously succeeds. I think it's worth considering adding a failure case to bin/eftest to fail if it can't detect any tests. What do you think?

--- a/bin/.eftest.clj
+++ b/bin/.eftest.clj
@@ -31,5 +31,10 @@
                      [(when junit?
                         (report-to-file
                           (requiring-resolve 'eftest.report.junit/report)
-                          junit-path))]))})]
-  (System/exit (+ (:error summary) (:fail summary))))
+                          junit-path))]))})
+      no-tests-discovered? (zero? (:test summary))]
+  (if no-tests-discovered?
+    (System/exit 1)
+    (System/exit (min (+ (:error summary) (:fail summary))
+                      ;; Exiting with higher than 255 wraps back around to 0 and starts counting again.
+                      255))))

[SevereOverfl0w]

Could we instead use eftest's find-tests directly by passing it "test" for the test directory? It's not fully general, but it seems like it could be the kind of useful default that edge provides.

This would generalize by applying find-tests with the project directories that orchard would scan. Orchard searches the classpath for directories relative to (System/getProperty "user.dir"). The code is reasonably small, could be done quite easily.

Zooming out further, this isn't the first time I've hit this class of bug. I've had other cases in the past where no tests can be detected due to a bigger problem, and the test run erroneously succeeds. I think it's worth considering adding a failure case to bin/eftest to fail if it can't detect any tests. What do you think?

This feels "off" technically. In the same way that having map throw an exception for an empty list would be "off". 0/0 is successful. However, I'm struggling to think of reasons not to do this. What follows is a running consciousness of the implications.

The use-case I could think of for not doing this was on brand-new projects with some kind of in-place CI which starts failing for not writing any tests. Maybe that would be more likely in the case of a cljs-only project, as eftest might be run unconditionally (e.g. against all directories). Grasping at straws there though.

I've also hit this class of issue though too. I can't think of a case where it wasn't due to a syntax error, so maybe avoiding orchard's ignore-invalid-nss is sufficient.

In cases where filtering is being performed (e.g. for metadata or matching a regex) what would be the expected behavior if no matching tests were found? If I was writing something which did find -type d -exec '../bin/eftest --include :integration' and one project didn't have any integration tests, I personally would expect that to still succeed.

One benefit of Orchard's ignoring of invalid namespaces is that it means that when you have some known-invalid clojure files (e.g. for documentation purposes) then you can easily ignore them.

It might be that projects should have a "syntax" script for detecting these cases. Sure 0/0 of the tests passed, but look, a big red box next to syntax! Syntax might have it's own mechanism for indicating a syntax failing directory to be ignored (.syntax-ignore or something). This essentially moves the problem elsewhere, but does a good job of isolating the concern. It's a different problem to solve when thinking about solving the syntax case when it's free of the test attachment.

The case of tests might be more clear cut (0/0) but there are other kinds of check that may be more difficult to ascertain clearly.

---

I'd be curious if this elicits any comments from you, and for your feelings on the separate syntax check.

[danielcompton]

This feels "off" technically. In the same way that having map throw an exception for an empty list would be "off". 0/0 is successful.

Yep, I know what you mean here.

One benefit of Orchard's ignoring of invalid namespaces is that it means that when you have some known-invalid clojure files (e.g. for documentation purposes) then you can easily ignore them.

I think you'd normally want these known-invalid Clojure files to be in a separate documentation directory, or somewhere not on the class path?

If I was writing something which did find -type d -exec '../bin/eftest --include :integration' and one project didn't have any integration tests, I personally would expect that to still succeed.

Perhaps if this was to be added, you could also have an --allow-zero-tests? flag. This would disable the zero check in cases where you wanted to indiscriminately run the same script on all subdirectories.

I've also hit this class of issue though too. I can't think of a case where it wasn't due to a syntax error, so maybe avoiding orchard's ignore-invalid-nss is sufficient.

The other times I can remember hitting the 0/0 tests issue is when something was partially refactored, commented out, or otherwise typoed, but was still in a state where the test runner could succeed. This is a smaller concern for me though.

It might be that projects should have a "syntax" script for detecting these cases.

This seems possible to me, but I'm not sure the benefits of a separate test run outweigh just having the test runner fail if it can't compile a file. It would add another point of failure, and make tests take longer to complete.

your feelings on the separate syntax check.

I can't really think of any use-cases here that wouldn't be served by compiling all namespaces in the test run, but maybe you have some you're thinking of?

[SevereOverfl0w]

This may have been mentioned, but I don't think it was.

I just encountered a use case where you have optional namespaces in your application which may not load unless certain optional dependencies are added. For example, a kaocha reporter.

[SevereOverfl0w]

I'm thinking that 0/0 is actually insufficient. It's possible that a single namespace might fail to load, but 99% of them do, meaning that most tests are loaded.

I'm thinking the solution is allowing a white/blacklist for files to search. The default of searching the cp is good, but allowing an override on a per-directory (adjacent to deps.edn I suppose) basis would also be good.

Then the behaviour can be changed to throw on invalid namespaces, but you can still do easy automation.

We're getting into the territory of writing a full test runner here, so we should perhaps consider whether there's now mature solutions and ../bin/eftest should be retired, or to spin this out as something that's generically useful.

Notably, ../bin/eftest doesn't allow access to a lot of the flags that are exposed by eftest, e.g. parallelism on/off.