Add testing conventions

lvh commented 8 years ago

Originally from #131. #132 closed that and added test naming conventions, but there are other test conventions that we should talk about:

[ ] using testing to provide context for what you're testing
[ ] order of expected and computed value for (is (= ...))

(Please don't hesitate to provide other suggestions so they can be discussed here.)

arrdem commented 8 years ago

Preference of are for multiple is examples.

bbatsov commented 8 years ago

Some ideas that we might borrow (in abstract form) for RSpec's best practices http://betterspecs.org/

jcf commented 8 years ago

The recent addition of how to name test vars is starting to move beyond how best to layout your code (which is what I'd consider a style guide should really focus on) and into how to write Clojure effectively.

There's no arguing that long lines make use of tooling more difficult, trailing whitespace is a pain, getting indentation wrong makes the next person's life harder, but sticking -test on the end of my test vars, or using the unnecessarily verbose RSpec DSL-style of testing is getting into more nuanced and complicated areas where defining rules becomes tricky.

To take @arrdem' example of using are for multiple uses of is, I agree that can work very well with simple test cases.

(are [k v] (= v (get m k ::missing))
  :name "James"
  :age 100
  :location "GitHub textarea")

But, I use are above primarily because it can be difficult to spot the issue when dealing with large data structures. Often, property-based testing is more appropriate where someone might be using are. There are occasions where use of are results in test failures that are hard to understand, and often a doseq with a descriptive error message passed to is can be much easier to work with.

(doseq [[kseq v] expected-vals]
  (is (= v (get-in m kseq ::missing))
      (str "Path " kseq " in:\n" (with-out-str (clojure.pprint/pprint m))))))

Writing tests so they're easy to understand and fix is more complicated than appending -test or adding a testing.

In terms of RSpec-style, which I've used a lot in the past, it often makes things more verbose than is really necessary.

describe Person do
  describe :name do
    context 'with no last name' do
      subject(:person) { Person.new(first: "James") }
      its(:name) { "James" }
    end
  end
end

(deftest t-person-name
  (is (= "James" (make-person {:first "James"}))))

The only time I really find myself using testing is when I have a group of test assertions that I want to attribute a name to. Often when I have a few tests driven from a collection of inputs for example, which is nothing like what BDD evangelists used to recommend.

(Bear in mind, the use of context and naming of RSpec tests came from the ability to print formatted specs when you ran your tests - something we don't have with clojure.test.)

Sorry for the lengthy brain dump. I'm just a little concerned that the style guide I often refer people to is overreaching a little. Some things are hard to define concrete rules for - they're nuanced, and as such require experience and experimentation to get right.

P.S. I use t-something and not something-test, which I see you recently labelled as bad? I don't think there's anything wrong with my style - it's consistent, and easy to search a codebase for.

lvh commented 8 years ago

I think your comment is an excellent example of why the style guide should include such things :) If it stuck to layout, it would be like cljfmt but worse, since it can't automagically fix and detect problems for you. This way it can effectively provide value in a way only human prose can. This is also consistent with style guides I've seen for other languages, both programming and human.

With regards to t- vs -test, you mentioned that it's consistent and easy to search for. However,

-test seems just as easy to search for as t-.
When you say consistent, you appear to mean consistent with your own codebase(s). Making a median codebase easier to pick up by other Clojure programmers is one the goals of the style guide. As such, using t- makes it harder for me to find a test in your codebase. It may be internally consistent, but that might also be said for e.g. long lines (just get your editor to soft-wrap correctly), weird identifiers (just get your editor to enter greek characters), weird indentation...

... so, and I hope you don't take a comment about codebases personally, but yeah, for the reasons we have a style guide anyway, t- is worse than -test. That appears to be the consensus from an overview of codebases. Yes, some people might do it differently now, but if everyone already did what the style guide said anyway, then we wouldn't need a style guide at all :)

I agree that I have only rarely used testing (and I'm not always sure it's a great idea, I'm a very soft +1 on it usually), but that looks like a distinct argument from what the style guide should be about.

arrdem commented 8 years ago

I mean... if you wanna get really nitpicky #"^.*?-test$" is poor style since it's a postfixing convention which should be fully folded into the namespace's name. Using qualified refers to avoid name conflicts (general good style) would be "best" and imposes no naming convention on tests.

There are definitely -test and .test. conventions for test namespaces running around already.

jcf commented 8 years ago

... so, and I hope you don't take a comment about codebases personally, but yeah, for the reasons we have a style guide anyway, t- is worse than -test.

@lvh Thanks for taking the time to respond to my comment. I don't take it personally, and my feelings most certainly aren't hurt. 😄

As such, using t- makes it harder for me to find a test in your codebase.

I think this point can be refuted pretty quickly.

$ tree test
test
└── oauth
    └── two_test.clj

1 directory, 1 file

$ ag deftest test
test/oauth/two_test.clj
35:(deftest t-make-valid-client
100:(deftest t-authorization-url
139:(deftest t-access-token-request

Finding tests hasn't ever caused me an issue. More often that not, each test file corresponds to an implementation file, and the test names are pretty descriptive. Even when the test files are all over the place they have to be on the classpath at least!

Now as for understanding the tests… well that's a different matter entirely! 😉

That appears to be the consensus from an overview of codebases.

I'm not sure this is accurate given you haven't cited your source/research. We could do some analysis of code in the wild to support your claim that we'd be within the median of typical Clojure code, but in all honesty I don't consider safety in numbers to be much of a justification for one approach vs. another.

Many forms exist in the wild, and I can easily churn out a few hundred repos of questionable quality Clojure code to skew your metrics.

The Clojure codebase features use of -test, test, and naked test vars, and it works excellently as I'm sure we'd all agree.

Making the process of software development easier is the only thing I want a style guide for. Consistency, meaningful debates instead of bike shedding, and a guide for beginners is what I want to provide through a style guide.

The t- vs. -test argument still looks to be entirely subjective to me at this point. I am of course open to being convinced otherwise. Strong opinions loosely held!

bbatsov / clojure-style-guide

Add testing conventions #133