INRIA / INRIA/spoon

How should we specify test method contracts?

Open
#3,984 11 comments 0 reactions 0 assignees View on GitHub
discussion
Dominant language
Java
Stars
2k
Forks
392
Avg merge
11h 24m
Merged PRs (30d)
36

Description

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).

```java
@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):

```java
@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!

```java
@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?

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.