Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ContextB application in cleared #255

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,10 @@ public void testContextualConsumer() throws Throwable {
* A ContextService contextualizes a Function, which can be supplied as a dependent stage action
* to an unmanaged CompletableFuture. The dependent stage action runs with the thread context of
* the thread that contextualizes the Function, per the configuration of the ContextServiceDefinition.
*
* Assertions on results[0] and results[1] are both invalid because treating those two UNCHANGED context types as
* though they were CLEARED.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems out of place given that this pull is attempting to correct the assertion on results[0] to make it valid.

* TCK challenge: https://github.com/jakartaee/concurrency/issues/253
*/
@Test
public void testContextualFunction() throws Throwable {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@
cleared = StringContext.NAME,
unchanged = TRANSACTION)
@ContextServiceDefinition(name = "java:module/concurrent/ContextB",
cleared = TRANSACTION,
unchanged = { APPLICATION, IntContext.NAME },
cleared = {TRANSACTION, APPLICATION},
unchanged = { IntContext.NAME },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this change would only correct the assertion on APPLICATION context and not the subsequent assertion on IntContext. Also, is it safe with respect to all of the other TCK tests that use "java:module/concurrent/ContextB" to alter this configuration? It seems like the safest change would be to either adjust the assertions to match the specified annotation values, or otherwise create an additional ContextServiceDefinition with values that the assertions expect.

propagated = ALL_REMAINING)
@ContextServiceDefinition(name = "java:comp/concurrent/ContextC")
@WebServlet("/ContextServiceDefinitionServlet")
Expand Down Expand Up @@ -407,7 +407,7 @@ public void testContextualFunction() throws Throwable {

Object[] results = future.get(MAX_WAIT_SECONDS, TimeUnit.SECONDS);
assertTrue(results[0] instanceof NamingException,
"Application context must remain unchanged on contextual Function " +
"Application context must be cleared on contextual Function " +
"per java:module/concurrent/ContextB configuration. Result: " + results[0]);
assertEquals(results[1], Integer.valueOf(0),
"Third-party context type IntContext must remain unchanged on contextual Function " +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public void testAsynchronousMethodVoidReturnType() {
}

// TCK Accepted Challenge: https://github.com/jakartaee/concurrency/issues/224
@Test(enabled = false)
@Test
public void testCompletedFuture() {
runTest(baseURL);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ public void testCompletedFutureMSE() throws Throwable {

try {
String result = stage2.join();
throw new AssertionError("Application context must be left unchanged per " +
throw new AssertionError("Application context must be cleared per " +
"ManagedExecutorDefinition and ContextServiceDefinition config. " +
"Instead, was able to look up " + result);
} catch (CompletionException x) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public void testAsynchronousMethodWithMaxAsync3() {
runTest(baseURL);
}
// Accepted TCK Challenge: https://github.com/jakartaee/concurrency/issues/224
@Test(enabled = false)
@Test
public void testCompletedFutureMSE() {
runTest(baseURL);
}
Expand Down