Skip to content

Conversation

@aubi
Copy link
Contributor

@aubi aubi commented Jul 11, 2022

In the tests, ContextB's APPLICATION is tested to be cleared. This PR changes it from unchanged to cleared and enables all related challenged tests.

@aubi aubi requested a review from njr-11 July 11, 2022 10:16
Copy link
Contributor

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

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

I added a couple of comments. Please disregard my earlier comment about needing to find a different follow-up reviewer. It looks like I'll be working this week with the other plans now cancelled due to COVID still present in the household, so I should be able to review updates after all.

* 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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants