Removing Tests of Context Switch from Asynchronous Tests

[aubi]

I removed tests of context switch, which tested, that the context capture happened during jndi lookup. The test later setup number 22 and the test checks, if asynchronous method (running via scheduler) returns 0.

The code:

ManagedExecutorDefinitionFullTests.testScheduledAsynchIgnoresMaxAsync() {
...
            IntContext.set(22); //Context should be cleared
            CompletableFuture<Integer> future = reqBean.scheduledEvery3Seconds(1, counter);
...
            assertEquals(Integer.valueOf(0), future.get(MAX_WAIT_SECONDS, TimeUnit.SECONDS),
                    "ManagedScheduledExecutorService with maxAsync=4 must be able to run scheduled async methods concurrently.");

reqBean.scheduledEvery3Seconds runs only once and returns IntContext: future.complete(IntContext.get())

1. There is no explicit setup of the IntContext value to test.
2. This test checks just the context switch, not asynch methods running concurrently as it says in the message. Although it is called schedule, it runs only once.

The discussion, if the context capture should happen on JNDI lookup or when the thread is created, is still open: #253

[njr-11]

2. This test checks just the context switch, not asynch methods running concurrently as it says in the message. Although it is called schedule, it runs only once.

This is not true. Notice that the name of the test method is "testScheduledAsynchIgnoresMaxAsync" indicating that it is testing that a Scheduled Asynchronous method runs even if the max async is used up by other operations. By removing the line to request the asynchronous method and removing the assertion of its completion, the test becomes useless and no longer tests what it was written for.

The discussion, if the context capture should happen on JNDI lookup or when the thread is created, is still open: #253

I don't see discussion of that issue under #253 . In any case, the discussion/debate is about context capture for ManagedThreadFactory, not ManagedExecutorService and ManagedScheduledExecutorService. The former is documented with a different behavior for context propagation than the latter. I am not aware of issues over the latter, but in any case it makes no difference whatsoever to this pull because the executors in question are not even configured to propagate the IntContext.

The executor here is java:module/concurrent/ExecutorB:

    @Asynchronous(executor = "java:module/concurrent/ExecutorB", runAt = @Schedule(cron = "*/3 * * * * *"))
    public CompletableFuture<Integer> scheduledEvery3Seconds(final int runs, final AtomicInteger counter) { 

Its configuration shows it to be using the java:module/concurrent/ContextB ContextService.

@ManagedExecutorDefinition(name = "java:module/concurrent/ExecutorB", context = "java:module/concurrent/ContextB", maxAsync = 1)

java:module/concurrent/ContextB is actually defined differently in 2 different modules, elsewhere making for a very good test of java:module configuration.

In ContextServiceDefinitionBean,

@ContextServiceDefinition(name = "java:module/concurrent/ContextB", propagated = { APPLICATION,
        StringContext.NAME }, cleared = IntContext.NAME, unchanged = TRANSACTION)

In ContextServiceDefinitionServlet,

@ContextServiceDefinition(name = "java:module/concurrent/ContextB", cleared = TRANSACTION, unchanged = { APPLICATION,
        IntContext.NAME }, propagated = ALL_REMAINING)

In neither case is IntContext propagated. In the EJB module, it is cleared. In the web module, it is left unchanged. The real problem here appears to be that the author of the test was likely looking at the wrong one (the EJB module) when writing the assertion. If it were supposed to be cleared, the assertion of 0 would be correct. However, the configuration that applies is the one for the web module where IntContext must be left unchanged on the thread of execution. I would guess that most implementations would be running the method on a pooled thread that likely has has no IntContext and inadvertently end up with 0 allowing it to pass. However, there is no requirement of that and so the assertion for a value of 0 is wrong.

In order to correct this in a way that preserves the intent/usefulness of the test case, I am recommending that the assertions be switched to assertNotNull so that it asserts completion of the scheduled asynchronous method. The return value does not matter.

The suggestions that I added do not include adding an import statement for assertNotNull. I could not see way for git to allow me to suggest that, so it would need to be added separately.

[aubi]

I see your point, Nathan. Your suggestions look good, I'll send a commit in a second.

[aubi]

@njr-11 If you will agree with the changes, I'll merge the commits, so there is just one commit, not two doing remove/readd.

[njr-11]

@njr-11 If you will agree with the changes, I'll merge the commits, so there is just one commit, not two doing remove/readd.

It looks good now. Thanks for spotting that the tests had an issue here!