How should we specify test method contracts?

[slarse]

There is a (afaik unwritten) rule in Spoon that tests should start with an inline comment specifying the contract of the test. A lot of the time, this is useful as tests often recreate complex states to reproduce some bug under some incredibly specific circumstances. However, there are times when these comments are incredibly redundant.

For example, have a look at this test (inspired by real-life events).

@Test
void testConstructorThrowsExceptionWhenPassedNull() {
    // contract: The constructor of SomeClass should throw IllegalArgumentException when passed null
    assertThrows(IllegalArgumentException.class, () -> new SomeClass(null));
}

The contract reiterates what is already pretty clear from the method name itself, and very clear from the test method body. To me, the contract here is just noise that duplicates the test method logic. Duplication is bad. What if the type of the exception was changed? I can see this happening (contract and actual implementation diverge):

@Test
void testConstructorThrowsExceptionWhenPassedNull() {
    // contract: The constructor of SomeClass should throw IllegalArgumentException when passed null
    assertThrows(NullPointerException.class, () -> new SomeClass(null));
}

I don't disagree with having explicit contracts where they actually add some value, but in small and concise test methods I think the test and its name should speak for itself (and if it doesn't, it needs to be refactored until it does). In the example above, the explicit contract is just noise to me. I won't see it unless I delve into the source code, and if I'm in the source code the test implementation is in fact more concise and precise.

My personal preference would be to be more strict about well-named methods, as when a test fails, the name is what you see. Starting each test method name with test is also the height of redundancy: we know it's a test already!

@Test
void constructor_whenPassedNull_throwsException() { ... }

Often, a good name is enough. When it's not, a contract comment can be added.

But even barring a particular naming convention the contract is often superfluous, as in the example I outlined at the top.

Any thoughts on this?

[andrewbwogi]

This seems like a governance issue. It's easier to require comments on all tests than manually deciding for each test whether it's complex enough to warrant a comment. I agree with you that duplication is bad but if small examples as yours are not very frequent, I think it's worth the price. Then we should rather elevate test contracts from an unwritten rule to a requirement automatically checked.

There's always a risk that comment and code diverge when the code is changed. It can be mitigated by keeping the comment somewhat general. In your example it would suffice with:

// contract: The constructor should throw a correct exception when passed null

The test code then defines what "constructor" and "correct exception" actually mean. I think that's fine since the purpose of the comment is not a formal specification but the conceptual function of the test for human communication.

[slarse]

It's easier to require comments on all tests than manually deciding for each test whether it's complex enough to warrant a comment.

To me, this is the only feasible argument. Making things easier for maintainers is a completely valid argument. But this precludes that the contract comments are a good idea in general, and of this I am not convinced. Note also that only about half of the tests in Spoon even have contract comments, and so an architecture test at this point seems infeasible.

I have three arguments for why it's better to encode a contract into the method name, than in a mandatory contract comment:

1. The method name must be there, we can't not have it. Thus, the method name must be something and why not make it meaningful? In order to avoid duplication between the method name and the contract, we would have to require the method name not to be meaningful, which seems rather counter-productive.
2. The method name is what is shown when a test fails, not the contract comment.
3. The contract comment is specific to Spoon, I've never seen it used in any other project. While many developers know that meaningful names are good, only those who previously have worked on Spoon will actually know to put in a contract comment.
   • And yes, even if the contributing guidelines were to mention this, I've found that many people don't read the contributing guidelines

I agree with you that duplication is bad but if small examples as yours are not very frequent, I think it's worth the price.

The problem I see isn't just that it's duplicated, it's that it's often tripled. Once in the test method name, once in the contract essentially restating the test method name, and then once in the implementation.

but if small examples as yours are not very frequent,

This is actually another bad smell on the test suite: small tests aren't very frequent. There are a lot of tests that test way too many things and are way too large. I don't think arguing from the perspective that most tests are large is very strong, because that in itself is a bad sign.

Really, this discussion should perhaps be broader in the sense "we need to discuss the test suite", because it is undeniably somewhat messy. Adding a new test is confusing due to the seemingly haphazard organization of the tests. The sporadic use of contract comments is just one out of several issues, test resource placement and management being another major problem.

There's always a risk that comment and code diverge when the code is changed. It can be mitigated by keeping the comment somewhat general. In your example it would suffice with:

// contract: The constructor should throw a correct exception when passed null

This even more devalues the comment, because now there's literally nothing in the comment that is not present in the test's name. It's just noise.

The test code then defines what "constructor" and "correct exception" actually mean. I think that's fine since the purpose of the comment is not a formal specification but the conceptual function of the test for human communication.

I agree that this is fine. I think this also speaks against using the contract comment, as developers (myself included) tend to think less about what they write in a comment than how they name their methods. I really think that encouraging free-text contracts promotes overspecifying or even incorrectly specifying the contract from the beginning.

So, my proposition would still be to put a non-overspecified contract in the method name, and then if necessary add additional context in free-text.

[slarse]

Just for the record, I'm not saying I'm right. This is just my opinion, I'm open to be convinced otherwise.

At the very least, the end result of this discussion will be that we actually add the conclusion to the contributing guidelines. That way, we have something to reference when reviewing pull requests that don't follow whatever we agree on here!

[slarse]

Changed the title as I fell like it's not about if we should specify the test contract, but how.

[monperrus]

We want to have strong and meaningful contracts as much as possible. I'd say that the number of quality of specified contracts we have in the code base is already much higher than the average in good projects.

How should we specify test method contracts?

In the best possible way. If it works with the method name only, fine. But in some cases

1. It's not human readable
2. The method name encourages short contracts while real world contracts are often more verbose
3. It would prevent us packing different closely related contracts in a single meaningful test.

We are pragmatic, the most appropriate way depends on the contract length and the test scope design.

We are pragmatic, if we see a missing contract, we add it. If we see how to improve an existing contract, we improve it. We love pull-requests that improve tests, that add contracts, that improve existing contracts.

[andrewbwogi]

A central problem concerns duplication. I'd even say that there is no duplication here except in a shallow or accidental sense. The reason is that code and comments belong to separate categories of language that are essentially different, namely the formal and the conceptual. The former is adapted for machines, the latter entirely for humans. The fact that a line of code sometimes mimics the concept should not stop us from encouraging the explicit relating of concepts to algorithms.

I really think that encouraging free-text contracts promotes overspecifying or even incorrectly specifying the contract from the beginning.

Another viewpoint is that contract comments help us to think conceptually about tests and therefore about the components and their relationships. This is important because we mostly use concepts when communicating about software.

Note also that only about half of the tests in Spoon even have contract comments, and so an architecture test at this point seems infeasible.

We could make an exception for past tests but enforce contract comments for new ones.

[slarse]

@monperrus

How should we specify test method contracts?

In the best possible way. If it works with the method name only, fine. But in some cases

1. It's not human readable

That depends on how you write it. If it's testGetReferenceFromParameterizedBindingIsReferenceWithExpectedTypeParameters, then perhaps not. If it's instead written getReference_fromParameterizedBinding_isReferenceWithExpectedTypeParameters, I find it perfectly human readable, and a contract // contract: getReference returns a reference with the expected type parameters when passed a parameterized binding is no more informative.

2. The method name encourages short contracts while real world contracts are often more verbose

Another view is that it forces the writer to think more about the contract in order to express it concisely. Expressing something more verbosely does not necessarily make it more clear.

3. It would prevent us packing different closely related contracts in a single meaningful test.

No, because this misses a key part of my proposition: don't put in redundant contract comments.

We are pragmatic, the most appropriate way depends on the contract length and the test scope design. We are pragmatic, if we see a missing contract, we add it. If we see how to improve an existing contract, we improve it. We love pull-requests that improve tests, that add contracts, that improve existing contracts.

This is precisely my point. If the contract is simple enough to be specified by the method name, then the contract comment is superfluous noise. Even more, asking a contributor to provide a contract comment that is clearly redundant in a crystal clear 1-5 line test method is a bit awkward. I've done that on several occasions, and it doesn't feel particularly pragmatic.

@andrewbwogi

The reason is that code and comments belong to separate categories of language that are essentially different, namely the formal and the conceptual.

Absolutely, they are different categories of languages. Duplication is however not constrained by language. I'll give you an extreme example (I come across similar things a lot in code review)

// divide a by b and store the result in c
double c = a / b;

Again, this is an extreme example. But by your reasoning, there is no duplication here because different categories of languages are used. I vehemently disagree with that reasoning. Duplication transcends language. A programmer understands both the programming language, and natural language, so if the programming language can be made to speak for itself, it should. Note can: sometimes this is too hard, or simply not possible, and then we add a comment.

The fact that a line of code sometimes mimics the concept should not stop us from encouraging the explicit relating of concepts to algorithms.

I think that if code "sometimes mimics the concept", it's very poor code. Code should precisely define the concept. If it doesn't, it should be refactored. An inline comment that explains what the code does is most often a bad code smell. Sometimes a necessary one, but a code smell nonetheless.

We could make an exception for past tests but enforce contract comments for new ones.

AFAIK, our architecture tests don't have that capability. Someone would need to add in ratcheting.

[monperrus]

If the contract is simple enough to be specified by the method name, then the contract comment is superfluous noise.

Agree.

Even more, asking a contributor to provide a contract comment that is clearly redundant in a crystal clear 1-5 line test method is a bit awkward.

Agree. We want to encourage high effectiveness, good tests and good contracts.

[slarse]

My suggestion for the contributing guidelines is as follows:

A test case must have a contract specifying what the expected outcome is, and under what conditions. The contract must either be clearly evident from the test method name, or expressed in an inline contract comment. In general, if the contract paints up a complex situation and outcome, it is better put in an inline comment (common for end-to-end tests that start with the Launcher). If on the other hand the contract is rather simple, it may be appropriate to just put it in the method name (common for unit tests that test a highly isolated unit). We will never ask you to remove a contract comment unless it is incorrect, so when in doubt, feel free to specify the contract in a comment.

And then we add an example of each. This will make it easier to contribute. And for me, less painful to review a well-written and clear test :)

I don't think this in any way will add maintainer workload, because maintainers must still inspect the tests and verify the contracts. Thus, deciding whether the existing method name is sufficient or not is just part of the job.

In hindsight, I should have drafted a proposal like this from the beginning. I think you (@andrewbwogi @monperrus) got the idea that I wanted to forbid expressing test contracts in natural language. That IMO would be orders of magnitude worse than to require them everywhere, and was never my intention. I've written many tests with very elaborate contracts simply because the situation I needed to set up was incredibly specific, on the order of "when X happens on a rainy day in August and Jupiter aligns with Mercury, then parsing a type parameter in a generic type used in a catch clause causes situation Z, which ...".

@andrewbwogi As an addendum to my previous response: I think your ideas do have merit when it comes to tests, and I think my response might come off as completely rebutting them. It is sometimes extremely difficult to clearly express a test contract in just code, and then a natural language contract is absolutely necessary, even though it duplicates the code.

[andrewbwogi]

If it's instead written getReference_fromParameterizedBinding_isReferenceWithExpectedTypeParameters, I find it perfectly human readable, and a contract // contract: getReference returns a reference with the expected type parameters when passed a parameterized binding is no more informative.

I grant that the method name can replace the contract comment if it can be made short. But here it looks like the method name is doing something it was not designed for. The natural language contract is easier to read and aesthetically more pleasing to me.

[slarse]

@andrewbwogi Just an FYI my answer here is somewhat unrelated to the issue, as the conclusion of this issue is to not require a contract when it's obviously redundant. But I find the discussion interesting so here goes.

I grant that the method name can replace the contract comment if it can be made short.

I see no reason why a test method name should be short. As long as it doesn't break the line limit, it's fine to me.

But here it looks like the method name is doing something it was not designed for.

Not so. The whole point of naming a procedure is to communicate its purpose, so here the method name is doing exactly what it was designed for. Granted, Java came around before testing was a widespread practice, and so doesn't cater as well as it could to expressing test contracts in particular. More modern languages like Kotlin fix this by allowing identifiers to be clear text, but in older languages like Java we need to make due with what we have.

IMO, giving a test method the name test1 and then writing the contract somewhere else is more using the name for something it was not designed for. If I have 10 test failures and only see test1, test2, ..., test10, then finding a pattern will likely be hard. If the tests are carefully named, finding the connection between them will likely be less work.

The natural language contract is easier to read and aesthetically more pleasing to me.

I completely agree. If we could have only a clear text contract, and that would actually appear in test output, then it would be much better to use that. But we can't. The contract comment does not appear in test output, and the method name must be there, and therefore not putting meaning into the method name is a wasted opportunity. If the entire contract can be adequately captured in the method name, then the contract comment is unnecessary duplication.

Is the method name (in Java) harder to read than a clear text comment? Yes, of course. But in most cases, it can be made sufficiently readable. If a naming convention is adopted, it can often be even easier to read the contract from the method name because you know exactly where to expect which part of the contract (the same can of course be said about adopting a convention for contract comments).

All that said, is it feasible to expect the average contributor to be proficient enough in naming to capture anything but very simple contracts in method names? Probably not, it takes a lot of practice. But when a PR comes in such that the test method name does capture adequately the contract, it's just inconvenient and unnecessary work from maintainers to require a contract comment as well. Even if we don't agree in general on this whole discussion point, I think we can agree on that?