Catch stateful extensions at test time

[jbduncan]

As I've been developing #491, I've fallen into the trap where I added an instance field to my extension, only to realise later on that the JUnit 5 User Guide implies that state should be stored in an ExtensionContext.Store. (I don't know when extensions actually get reinstantiated, as my tests haven't caught this scenario, and the JUnit 5 User Guide doesn't seem to clarify this.)

So I believe we should introduce a test that checks that none of our extensions store state as instance fields. We could use ArchUnit, ClassGraph or reflection to do this.

Goal

Introduce a test that:

1. Finds all extensions in our codebase.
2. Selects any extensions that have an instance field.
3. If the number of these extensions is greater than zero, throw an AssertionError.

[jbduncan]

What does everyone think?

[beatngu13]

Hmmm, not sure if it is worth the effort of creating and maintaining automated tests for this. Usually, such things get caught in PR reviews.

But, if anyone disagrees and we want to implement this, I think a custom SonarQube rule would fit best – which is not possible with SonarCloud AFAIK. 😕

[Bukama]

I am with @beatngu13.

[jbduncan]

@beatngu13 @Bukama Fair enough, I understand!

Nicolai did catch my mistake in the first place, and having played around with creating an ArchUnit-based test just now (see below), it took me a while to figure out how to make it, and the test doesn't look that intuitive to me anyway. So I'll leave this open for a few more days in case anyone else disagrees, but feel free to close it anytime you like, both.

package org.junitpioneer;

import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes;
import static java.util.stream.Collectors.toList;

import com.tngtech.archunit.core.domain.JavaClass;
import com.tngtech.archunit.core.domain.JavaClasses;
import com.tngtech.archunit.core.domain.JavaField;
import com.tngtech.archunit.core.domain.JavaModifier;
import com.tngtech.archunit.core.importer.ClassFileImporter;
import com.tngtech.archunit.lang.ArchCondition;
import com.tngtech.archunit.lang.ConditionEvents;
import com.tngtech.archunit.lang.SimpleConditionEvent;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.Extension;

import java.util.List;

class ArchitectureTests {

	@Test
	void checkArchitecture() {
		JavaClasses javaClasses = new ClassFileImporter().importPackages("org.junitpioneer");
		ArchCondition<JavaClass> haveNoInstanceFields = new ArchCondition<JavaClass>("have no instance fields") {

			@Override
			public void check(JavaClass item, ConditionEvents events) {
				List<JavaField> instanceFields = item
						.getFields()
						.stream()
						.filter(field -> !field.getModifiers().contains(JavaModifier.STATIC)).collect(toList());
				events
						.add(new SimpleConditionEvent(null,
							instanceFields.isEmpty(),
							item + " should " + getDescription() + "but has " + instanceFields));
			}

		};
		classes()
				.that()
				.areAssignableTo(Extension.class)
				.should(haveNoInstanceFields)
				.because("JUnit Jupiter does not guarantee that instance state will persist between invocations of any "
						+ "extension; instead, 'ExtensionContext.Store' should be used")
				.check(javaClasses);
	}

}

[Bukama]

Thanks for the draft of the test. My only dislike is the name as I wouldnt expect that checkArchitecture checks that no extension is stateful but uses the ExtensionContextStore.

Aside that huge 👍

[jbduncan]

@Bukama Good to hear that you like the idea, at least. 😄

What if the test name were something like checkThatExtensionsUseExtensionContextStoreOverFields? Would that be clearer?

[nipafx]

I really like the rule and was super close to including it, but I'm hesitant to add - after compiler (warnings = errors), Checkstyle, Spotless, and SonarQube - a fifth tool that lints our code, particularly for only one check. If we come across more situations where ArchUnit is the solution and we end up including it, we can definitely have this rule as well.

[jbduncan]

That's totally fair, @nipafx! I'm glad that you liked this rule enough to almost include it, though. Indeed, let's wait until we have a need for ArchUnit (or ClassGraph or advanced reflection) elsewhere.