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

Clean up use of FHIRRequestContext in unit tests #3283

Closed
lmsurpre opened this issue Feb 3, 2022 · 2 comments
Closed

Clean up use of FHIRRequestContext in unit tests #3283

lmsurpre opened this issue Feb 3, 2022 · 2 comments
Assignees
Labels
enhancement New feature or request technical debt

Comments

@lmsurpre
Copy link
Member

lmsurpre commented Feb 3, 2022

Is your feature request related to a problem? Please describe.
We use a threadlocal to store important request context like the tenantId and dataStoreId.

In our tests, we often need to set the request context since its outside of the server and so the requestFilter doesn't set it for us.

The pattern is like this:

FHIRRequestContext context = FHIRRequestContext.get();
// do stuff with context
FHIRRequestContext.set(context);

or, equivalently

FHIRRequestContext context = FHIRRequestContext.get();
FHIRRequestContext.set(context);

// do stuff with context

However, because the object is already associated with the local thread when we call get(), there's no need to call set with it.

Describe the solution you'd like
Remove the unnecessary calls to FHIRRequestContext.set(context);

Describe alternatives you've considered
Finally move to CDI and have the request context injected all over

Acceptance Criteria

  1. GIVEN [a precondition]
    AND [another precondition]
    WHEN [test step]
    AND [test step]
    THEN [verification step]
    AND [verification step]

Additional context
Now that we don't allow any null values in the ThreadLocal (#3282), we can remove the null check any tests that are guarding against a null FHIRRequestContext.
Hint: look for if (context == null) {

@lmsurpre lmsurpre added enhancement New feature or request technical debt labels Feb 3, 2022
@punktilious
Copy link
Collaborator

On the other hand, it concerns me that a test is relying on a particular state but isn't doing anything to establish that state (or check that it's correct). Also, we may have some tests which are looking for null in order to set up the context, so as pointed out, it will be good to look for if (context == null) and examine the local logic.

lmsurpre added a commit that referenced this issue Feb 22, 2022
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Feb 22, 2022
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre lmsurpre self-assigned this Feb 23, 2022
@lmsurpre lmsurpre changed the title FHIRRequestContext.get() should automatically associate intialValues with the thread Clean up use of FHIRRequestContext in unit tests Feb 25, 2022
lmsurpre added a commit that referenced this issue Feb 25, 2022
…t wrapper

Thankfully I was mistaken in what I was seeing... I was thinking that the initialValue wasn't associated with the current thread (which made no sense)
but in reality I was observing sloppy handling of the FHIRRequestContext

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
Copy link
Member Author

these changes are pretty isolated to test code...will be tested as part of general regression testing before release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request technical debt
Projects
None yet
Development

No branches or pull requests

2 participants