Contextualise CF tck #197

FroMage · 2020-08-18T16:11:58Z

This is the TCK part of #173 whose code PR was merged.

I've added tests for the following things:

ManagedExecutor.copy(CF/CS) will propagate contexts
ManagedExecutor.getThreadContext() will return an instance with the same propagation settings as the ManagedExecutor
ManagedExecutor.newIncompleteFuture() (hoping all other methods behave the same, I didn't think it was worth testing them all, should I?) will respect the new ContextManager.Builder.withDefaultExecutorService()
ThreadContext.withContextPropagation(CF/CS) will respect the new ContextManager.Builder.withDefaultExecutorService(), and ban Async methods with no executor if not set, and allow them if set

Did I forget something?

njr-11

I tried out the TCK test on OpenLiberty's implementation, and there are a couple of problems, although after I made adjustments to the TCK locally, everything passed.

First, ManagedExecutorTest and ThreadContextTest are getting the following error:

java.lang.NoClassDefFoundError: org.eclipse.microprofile.context.tck.TckThreadFactory

This is because the test case uses the new TckThreadFactory class, but the class isn't part of the test application. It is easily fixed by adding,

in ManagedExecutorTest:

        return ShrinkWrap.create(WebArchive.class, ManagedExecutorTest.class.getSimpleName() + ".war")
                .addClass(ManagedExecutorTest.class)
                .addClass(org.eclipse.microprofile.context.tck.TckThread.class)
                .addClass(org.eclipse.microprofile.context.tck.TckThreadFactory.class)
                .addAsLibraries(fakeContextProviders);

and in ThreadContextTest:

        return ShrinkWrap.create(WebArchive.class, ThreadContextTest.class.getSimpleName() + ".war")
                .addClass(ThreadContextTest.class)
                .addClass(ThreadContextTest.class)
                .addClass(org.eclipse.microprofile.context.tck.TckThread.class)
                .addClass(org.eclipse.microprofile.context.tck.TckThreadFactory.class)
                .addAsLibraries(threadPriorityContextProvider, multiContextProvider);

After fixing the previous error, I see 3 occurrences of the following, which is due to the TCK requiring the optional ContextManagerProvider.getContextManagerBuilder method,

java.lang.UnsupportedOperationException
at org.eclipse.microprofile.context.spi.ContextManagerProvider.getContextManagerBuilder(ContextManagerProvider.java:155)
at org.eclipse.microprofile.context.tck.ManagedExecutorTest.withDefaultExecutorServiceIsUsedDirectlyAndViaGetThreadContext(ManagedExecutorTest.java

I made a note of this in each place. The UnsupportedOperationException just needs to be caught and the test skipped because the method is optional.

njr-11 · 2020-08-18T19:59:19Z

tck/src/main/java/org/eclipse/microprofile/context/tck/ManagedExecutorTest.java

+                .getContextManagerBuilder()
+                .withDefaultExecutorService(executorService)
+                .addDiscoveredThreadContextProviders()
+                .build();


ContextManagerProvider.getContextManagerBuilder() is optional. We need to allow for implementations that don't support it. I believe this can be done as follows:

ContextManager.Builder contextManagerBuilder; try { contextManagerBuilder = ContextManagerProvider.instance().getContextManagerBuilder(); } catch (UnsupportedOperationException x) { // getContextManagerBuilder is optional. Skip if unimplemented. return; } ContextManager contextManager = contextManagerBuilder .addDiscoveredThreadContextProviders() .withDefaultExecutorService(null) .build();

Ouch, I didn't realise that. But then how can I test the executor service in this case? I need to make sure that it works if it's specified and if it's set to null. If I use the default context manager I can't know.

If an implementation doesn't support getContextManagerBuilder, that means there is no way to configure the default executor service. The implementation will either have a built-in default that it uses for async dependent stages of withContextCapture, or it won't have a built-in default, which will mean UnsupportedOperationException is appropriate when attempting to create async stages. In order to test this, we can trap for UnsupportedOperationException on thenApplyAsync(fn), which is considered a passing result when getContextManagerBuilder is rejected, and in the absence of the exception, check that async dependent stages that are created from the copied stage run successfully, independent of any constraints that might be in place on the executor of the stage from which it was originally copied.

To clarify, your test is fine, except that it needs to trap UnsupportedOperationException and skip itself in that case. And then a separate test can be written to cover the default context manager, in which the test will need to trap UnsupportedOperationException on thenApplyAsync (or whatever *Async method is used)

OK, fine, is this better then?

Mostly, but two other issues were introduced in the changes. I added comments for those inline instead of under this conversation.

njr-11 · 2020-08-18T20:23:37Z

tck/src/main/java/org/eclipse/microprofile/context/tck/ThreadContextTest.java

+        ExecutorService executorService = Executors.newSingleThreadExecutor(new TckThreadFactory());
+        ContextManager contextManager = ContextManagerProvider
+                .instance()
+                .getContextManagerBuilder()


Same as previous comment - getContextManagerBuilder is optional, so test needs to skip if not implemented.

njr-11 · 2020-08-18T20:23:50Z

tck/src/main/java/org/eclipse/microprofile/context/tck/ThreadContextTest.java

+            throws ExecutionException, InterruptedException, TimeoutException {
+        ContextManager contextManager = ContextManagerProvider
+                .instance()
+                .getContextManagerBuilder()


Same as previous comment - getContextManagerBuilder is optional, so test needs to skip if not implemented.

njr-11

See inline comments for remaining 2 issues that were introduced in the fix. Also one minor comment that I forgot to mention yesterday - the copyright year should include 2020 on files.

njr-11 · 2020-08-21T21:25:50Z

tck/src/main/java/org/eclipse/microprofile/context/tck/ManagedExecutorTest.java

+    public void withDefaultExecutorServiceIsUsedDirectlyAndViaGetThreadContext()
+            throws ExecutionException, InterruptedException, TimeoutException {
+        ExecutorService executorService = Executors.newSingleThreadExecutor(new TckThreadFactory());
+        ContextManager contextManager = ThreadContextTest.getContextManagerBuilderIfSupported()


This fails with

java.lang.NoClassDefFoundError: org.eclipse.microprofile.context.tck.ThreadContextTest

To fix, just need to add ThreadContextTest.class under addClasses

Ooops, done.

njr-11 · 2020-08-21T21:41:41Z

tck/src/main/java/org/eclipse/microprofile/context/tck/ThreadContextTest.java

+                    .build();
+        } catch (SkipException x) {
+            // not supported, use the default and assume that one does not have a default executor service
+            contextManager = ContextManagerProvider.instance().getContextManager();


This is not a valid assumption. OpenLiberty's default/built-in context manager does have a default executor service. There is nothing in the spec that would prohibit this and force the assumption to hold, and so the test cannot require it. The Assert.fail lines that follow should be replaced with checking that the async stage actually does complete successfully.

I'm not following. I thought the behaviour of ThreadContext.withContextCapture(CompletionStage) used to be that *Async methods should throw, and should keep on throwing if we can't specify a default executor.

Are you saying that OpenLiberty will make them not throw now?

If that is the case, then I can't check at all whether the Async methods throw or not with OpenLiberty because I can't predict whether or not there will be a default executor and so both options would be fine depending on implementation, right?

I'm not following. I thought the behaviour of ThreadContext.withContextCapture(CompletionStage) used to be that *Async methods should throw

You are correct about what the requirement used to be.

and should keep on throwing if we can't specify a default executor.

The JavaDoc isn't worded that way. It states that apart from the ManagedExecutor.getThreadContext path, the new completion stages will use "the default executor service if provided by the platform".

Are you saying that OpenLiberty will make them not throw now?

Yes. OpenLiberty does provide a default executor service and therefore uses it here. The default executor service happens to not be switchable, but that is beside the point. While it would be possible to change the spec to prohibit this, I cannot see why we would change it to prohibit because allowing customers to use the *Async methods is the most useful behavior.

If that is the case, then I can't check at all whether the Async methods throw or not with OpenLiberty because I can't predict whether or not there will be a default executor and so both options would be fine depending on implementation, right?

Correct. Both options are valid for implementations, so a test needs to consider both to be passing results. It's still worth keeping the test because it will verify there are no failures at other points.

OK, thanks. Now I get it. I've updated the test.

njr-11 · 2020-08-21T21:43:41Z

tck/src/main/java/org/eclipse/microprofile/context/tck/ManagedExecutorTest.java

+                .getContextManagerBuilder()
+                .withDefaultExecutorService(executorService)
+                .addDiscoveredThreadContextProviders()
+                .build();


Mostly, but two other issues were introduced in the changes. I added comments for those inline instead of under this conversation.

ManagedExecutor.copy ManagedExecutor.getThreadContext ContextManager.Builder.withDefaultExecutorService

…t run async actions without executor Because it really depends if there is a default executor, and we have tests for both cases elsewhere (with/without)

njr-11

Looks good, and the new tests are all passing now.

FroMage · 2020-09-01T08:07:03Z

Shall I merge it then?

njr-11 · 2020-09-01T13:42:20Z

Shall I merge it then?

It all looks good to me and I'm in favor of merging. It looks like you also have @manovotn listed as a reviewer.

FroMage · 2020-09-01T13:49:12Z

We can wait for @manovotn

manovotn

Sorry, just forgot to green-stamp this one.

FroMage · 2020-09-02T09:57:47Z

Thanks.

FroMage requested review from manovotn and njr-11 August 18, 2020 16:11

This was referenced Aug 18, 2020

Adding context to existing CS/CF instances #173

Closed

Fix a TCK regression smallrye/smallrye-context-propagation#195

Merged

njr-11 requested changes Aug 18, 2020

View reviewed changes

FroMage force-pushed the contextualise-cf-tck branch 2 times, most recently from af04c29 to a28ecd0 Compare August 21, 2020 15:00

njr-11 requested changes Aug 21, 2020

View reviewed changes

FroMage force-pushed the contextualise-cf-tck branch from a28ecd0 to cf3db1e Compare August 24, 2020 12:08

FroMage added 2 commits August 25, 2020 10:16

Added tests for the latest new methods

710aab7

ManagedExecutor.copy ManagedExecutor.getThreadContext ContextManager.Builder.withDefaultExecutorService

Removed checks that ThreadContext.withContextPropagation(CS/CF) canno…

dcdeb54

…t run async actions without executor Because it really depends if there is a default executor, and we have tests for both cases elsewhere (with/without)

FroMage force-pushed the contextualise-cf-tck branch from cf3db1e to dcdeb54 Compare August 25, 2020 08:17

njr-11 approved these changes Aug 25, 2020

View reviewed changes

FroMage mentioned this pull request Sep 1, 2020

SmallRye Context Propagation 1.0.16 quarkusio/quarkus#11410

Merged

manovotn approved these changes Sep 2, 2020

View reviewed changes

FroMage merged commit 1098b57 into eclipse:master Sep 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Contextualise CF tck #197

Contextualise CF tck #197

FroMage commented Aug 18, 2020

njr-11 left a comment

njr-11 Aug 18, 2020

FroMage Aug 19, 2020

njr-11 Aug 20, 2020

njr-11 Aug 20, 2020

FroMage Aug 21, 2020

njr-11 Aug 21, 2020

njr-11 Aug 18, 2020

njr-11 Aug 18, 2020

njr-11 left a comment

njr-11 Aug 21, 2020

FroMage Aug 24, 2020

njr-11 Aug 21, 2020

FroMage Aug 24, 2020

njr-11 Aug 24, 2020

FroMage Aug 25, 2020

njr-11 Aug 21, 2020

njr-11 left a comment

FroMage commented Sep 1, 2020

njr-11 commented Sep 1, 2020

FroMage commented Sep 1, 2020

manovotn left a comment

FroMage commented Sep 2, 2020

Contextualise CF tck #197

Contextualise CF tck #197

Conversation

FroMage commented Aug 18, 2020

njr-11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njr-11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njr-11 left a comment

Choose a reason for hiding this comment

FroMage commented Sep 1, 2020

njr-11 commented Sep 1, 2020

FroMage commented Sep 1, 2020

manovotn left a comment

Choose a reason for hiding this comment

FroMage commented Sep 2, 2020