`TestWatcher` can no longer access data in `ExtensionContext.Store`

[joerg1985]

After upgrading from 5.10.2 to 5.11.0 a org.junit.jupiter.api.extension.TestWatcher can no longer access store data. Instead, it now fails with org.junit.jupiter.api.extension.ExtensionContextException: A NamespacedHierarchicalStore cannot be modified or queried after it has been closed.

In my mind the intention of the store is to allow extensions to attach data to the context, so this data should be accessible at least until all extensions related to the context are processed.

Steps to reproduce

import org.junit.jupiter.api.extension.BeforeTestExecutionCallback;
import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.api.extension.TestWatcher;

class Mock extends TestCase.Handling<TestCase<?, ?>> implements BeforeTestExecutionCallback, TestWatcher {

    static final ExtensionContext.Namespace PACKAGE_PRIVATE = ExtensionContext.Namespace.create(new Object());

    @Override
    public void beforeTestExecution(ExtensionContext context) throws Exception {
        ExtensionContext.Store store = context.getStore(PACKAGE_PRIVATE);

        store.put("value", new Object());
    }

    @Override
    public void testSuccessful(ExtensionContext context) {
        ExtensionContext.Store store = context.getStore(PACKAGE_PRIVATE);

        store.get("value");
    }

}

Context

• Used versions (Jupiter/Vintage/Platform): 5.11.0
• Build Tool/IDE: maven 3

[sbrannen]

Hi @joerg1985,

Thanks for raising the issue.

I have confirmed that the following all-in-one test class prints Value: enigma on 5.10.x and throws the aforementioned ExtensionContextException on main (5.11.x).

package example;

import example.TestWatcherTests.DemoTestWatcher;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.BeforeTestExecutionCallback;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.api.extension.ExtensionContext.Namespace;
import org.junit.jupiter.api.extension.ExtensionContext.Store;
import org.junit.jupiter.api.extension.TestWatcher;

@ExtendWith(DemoTestWatcher.class)
class TestWatcherTests {

	@Test
	void test() {
	}

	static class DemoTestWatcher implements BeforeTestExecutionCallback, TestWatcher {

		private static final Namespace NAMESPACE = Namespace.create(DemoTestWatcher.class);

		@Override
		public void beforeTestExecution(ExtensionContext context) throws Exception {
			getStore(context).put("value", "enigma");
		}

		@Override
		public void testSuccessful(ExtensionContext context) {
			System.err.println("Value: " + getStore(context).get("value"));
		}

		private static Store getStore(ExtensionContext context) {
			return context.getStore(NAMESPACE);
		}
	}

}

We will discuss this within the team.

[sbrannen]

As for why this happens, org.junit.platform.engine.support.hierarchical.NodeTestTask.cleanUp() is invoked before org.junit.platform.engine.support.hierarchical.NodeTestTask.reportCompletion().

NodeTestTask.cleanUp() closes the ExtensionContext.Store, and NodeTestTask.reportCompletion() invokes TestMethodTestDescriptor.nodeFinished(...) which invokes TestWatcher callbacks.

[sbrannen]

One option to fix the regression would be to remove the rejectIfClose() calls in the following "get" actions, and I verified that the DemoTestWatcher works as expected on main with that change.

https://github.com/junit-team/junit5/blob/5f9eeb714637ab641a13ff92fb840c6479b39690/junit-platform-engine/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java#L142

https://github.com/junit-team/junit5/blob/5f9eeb714637ab641a13ff92fb840c6479b39690/junit-platform-engine/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java#L159

That would effectively change the semantics of "closed" to "read-only".

However, we'd still need to keep the rejectIfClose() call for the two getOrComputeIfAbsent() variants, since computing a new value would result in a "write" to the closed store.

---

In summary, I think we should either change the semantics of "closed" to "read-only" or revert #3614.

Though, now that I think about it, the title of #3614 is "NamespacedHierarchicalStore should fail on modifications after close"; however, we actually implemented the checks for "reads" in addition to "writes" (modifications). So, changing the semantics of "closed" to "read-only" would align with the title (and perhaps the original intent) of that issue.

[sbrannen]

Though, now that I think about it, the title of #3614 is "NamespacedHierarchicalStore should fail on modifications after close"; however, we actually implemented the checks for "reads" in addition to "writes" (modifications). So, changing the semantics of "closed" to "read-only" would align with the title (and perhaps the original intent) of that issue.

@leonard84, would changing the semantics of "closed" to "read-only" meet your needs (and original intent) as well?

[marcphilipp]

Team decision: Make it read-only by removing rejectIfClosed() from the get methods and moving it into the function application in getOrComputeIfAbsent.