`ert-runner/run` improvements

[camdez]

I ran across a couple behaviors today that surprised me and I decided to "fix" them. Hopefully you'll agree that this is an improvement. ;) Viz.:

1. Only test files in the top level of the test directory are run. This clashes with the common testing convention of mirroring the source tree structure in the test directory. Patch causes the test directory to be recursively listed for tests.
2. When directly calling, say, cask exec ert-runner with a list of file paths, these files are silently ignored if they aren't in the test directory or if they don't end with -test.el. Patch changes the behavior so that every file explicitly-named is evaluated (regardless of name or location). Directories are recursively listed and *-test.el files are evaluated.

[camdez]

Whoops, realized I missed a necessary README change (f0714eb834869ff7f71421f02905f18a704ec7a5)—fixed now. That README update highlights what is likely the most significant behavioral change of this PR. Despite the backwards incompatibility, I still think these changes make a lot of sense. Would love to discuss that if you disagree. Cheers!

[swsnr]

This clashes with the common testing convention of mirroring the source tree structure in the test directory.

I don't think that this is a common convention in Emacs Lisp. Emacs Lisp projects typically don't have a nested structure anyway.

Patch causes the test directory to be recursively listed for tests.

I wonder what project you really need this for. Mind to share a link?

[camdez]

I don't think that this is a common convention in Emacs Lisp. Emacs Lisp projects typically don't have a nested structure anyway.

I don't necessarily disagree (lack of proper namespacing makes hierarchically-structured code difficult), but this is a convention seemingly in use by most of the testing world.

Even excepting a test directory structure that mirrors a code structure, I suspect that most ert-runner users would we quite surprised to find that if they divided their tests into test/unit and test/integration and then ran cask exec ert-runner, nothing would run at all:

$ find test 
test
test/test-helper.el
test/unit
test/unit/commands-test.el
$ cask exec ert-runner

Ran 0 tests in 0.000 seconds

(I know I was surprised.) Even Emacs itself has subdirectories of tests.

A logical next step might be to try and run the directory of tests directly:

$ cask exec ert-runner test/unit 

Ran 0 tests in 0.000 seconds

Nope. Ok, maybe I can just run my tests one file at a time...?

$ cask exec ert-runner test/unit/commands-test.el 

Ran 0 tests in 0.000 seconds

$ cask exec ert-runner do you even work?

Ran 0 tests in 0.000 seconds

:confounded:

I had to read the source to understand the behavior. Maybe others would do fine with the docs, but IMHO the current behavior would be much more intuitive with this patch. Thanks for your consideration—I'll be following this with a commit to name that lambda.

[rejeep]

@camdez I agree on your changes! I would like to see some tests for this, but as they are currently broken (#18) I can see that it would be difficult. So let's solve #18 first and then we get back to this.

[swsnr]

@camdez I don't think that “most of the testing world”—four examples, actually—bears any relevance to us. Emacs Lisp is different.

Your examples are all about languages with hierarchical namespaces and thus an inherently hierarchical source tree, which Emacs Lisp does not have. And at least three of your four examples (Haskell, Java, Clojure) don't even support running tests by files names. They all compile binary assets from test suites and run tests by some abstract name (e.g. class names or module names).

To be clear, I'm not against these changes. If they make sense to anyone who would like to use ert-runner this way, we should merge them.

But I don't think that this is the way to run ERT tests—by splitting them into different files and giving file names on the command line. ERT has its own powerful DSL to select tests by names, tags and even custom predicates, and ert-runner exposes a subset of it—tags and test name patterns—on the CLI via -t and -p.

IOW, don't run individual files via cask exec ert-runner test/integration/test-commands.el. Instead tag all your tests, and then use cask exec ert-runner -t integration,commands.

[rejeep]

I run single files quite often, only not nested apparently. The description for this project is "Opinionated Ert testing workflow", meaning we have a file structure in mind.

With this change, our opinion would be:

• Run ert runner on a directory and all files called -test.el will run, recursively.
• Run ert runner on a file and that file will run, no matter its name.

I think this is good.

[camdez]

@rejeep Totally agree RE #18. I had originally intended to add tests for this, why explains why I ended up filing #18. :)

@lunaryorn I wasn't trying to suggest that four examples fully represent every case in the testing world, by I do have to draw the line somewhere (and get back to work) rather than collecting examples all day. If a couple more would help, I'm happy to provide them, but no number seems like enough, ya know? :) It's not my intention to overgeneralize, I just (1) believe it is the case that most popular test frameworks work this way and (2) feel that these four examples present a nice variety of cases.

w.r.t. if those other testing frameworks have any relevance here...at the very least there's value presented by following an established idiom which makes the functionality intuitive to any person who has used any of those other frameworks. Of course that value could be trumped by some other unique value in the way it's already done, but frankly, the only reason I see to prefer the current behavior would be for backward compatibility. Which is important, but for a library at version v0.6.4, one still has leeway to focus on improving the tool, even in breaking ways.

Besides being more in line with existing conventions, this approach is also more expressive. So I think we simply need to balance the additional utility against the cost of breaking existing behavior. To be completely clear, these are the cases where existing behaviors would change (and, potentially, break someone's test scripts):

1. Explicit references to non-existent files would now yield errors instead of doing nothing. (Minimal importance, harmless.)
2. Test files in subdirectories of test/ would now when previously they would have been ignored. (Theoretically this could cause code to unexpectedly run, but if there's code in *-test.el files in test/ subdirectories, I suspect odds are decent the user thought that code was running. So this might be an improvement.)
3. The ability to run test files by name sans path (e.g. cask exec ert-runner foo-test.el) disappears. This would likely yield an error (file does not exist), but could invoke the wrong file if there happened to be a foo-test.el in the current directory. (One might consider this a bit of a regression but I think the new behavior is a lot more consistent and more flexible.)

As for our unfortunate lack of hierarchical namespaces, that's not the only viable use case for wanting subdirectories in your test/ directory. Literally any argument for categorization applies here, chief in my mind is some kind of unit / integration (or similar) split. I can't argue with your point that tags present a clean solution to running the appropriate tests in this case, but we could generalize the point and say "why even have a test/ directory at all? Surely we can find the appropriate files in the top-level of the project." Hierarchical file layout has its perks—many of them human in nature—and some which we probably haven't even thought of. Other test frameworks (e.g. RSpec) allow running by tag and / or filename. Is it critical? No. Do I use it? Definitely.

[rejeep]

Number three is not an issue. It was a mistake of me to add support for this in the first place. I'm just glad to see it gone.

[camdez]

@rejeep Sweet. I think it makes a lot of sense this way and I'm happy to see the silent "failures" (I accidentally named a file that didn't exist) gone.

If @lunaryorn has any additional concerns I'm happy to address them, otherwise I'll try to find a little time this evening to get some tests in place (alternately, if someone else wants to tackle it, I won't be getting to it for a few hours) and hopefully we can merge soon.

[swsnr]

It's ok. I just don't think that "everyone else in the testing" is an argument in and by itself for doing what everyone else in the testing world (supposedly) does, unless there's also a technical reason for doing it this way which actually applies to the case at hand. The testing world is a lot more diverse than you suggest, and Emacs Lisp is quite different from most other languages.

But as I said, I'm not against these changes. I do think that they manifest a wrong process, which is what I was trying to explain, but that doesn't stand against merging them, the more if it's the process that you prefer.

[camdez]

@lunaryorn Oof. I could argue with the first part of that but I don't think it's going to be constructive.

When you say "manifest a wrong process", you're referring to the option of using directories for organization rather than tags, correct? Is there something that makes this "wrong" beyond just the fact that it's different?

If you think using tags is a best practice, I'd have nothing against an addition to the README suggesting using the tag-based approach for most organization needs.

[camdez]

In fact, I think it's a fairly orthogonal concern, because the functionality I've removed (viz. filtering the set of tests run to those in the test/ directory which match a certain filename) already violates your idea of organizing / filtering by tags (only).

[camdez]

Ooooook, I rebased onto master so that the test suite would run properly and added a couple tests. I didn't explicitly test running tests outside of the test/ directory (say that three times fast...) because it would have required some slightly gnarlier changes than I wanted to make on my own (I'd have to add a new step to create files elsewhere, but then I'd have to augment the before-test process to delete files outside the test/ tree and I'm not sure what that should look like), but the gains seemed minimal over checking that we can run a non *-test.el file by name, which I did do.

Should be good to go!

[rejeep]

Merged, thanks! :+1:

[swsnr]

@camdez Thanks :)

[camdez]

@rejeep :beers: Thanks!

@lunaryorn Thank you for your feedback! It's good for the community to hash these things out.