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

issue6 Timeout tests #267

Merged
merged 1 commit into from Jan 20, 2020
Merged

issue6 Timeout tests #267

merged 1 commit into from Jan 20, 2020

Conversation

mmusgrov
Copy link
Contributor

@mmusgrov mmusgrov commented Jan 16, 2020

#6

The issue called for 3 different tests. I implemented the first one TckTests#timeLimitWithPreConditionFailed
The second test is very similar to TckTests#timeLimit so I have not added a one for that.
The third test that was asked for was to verify that the time limit is respected after a system crash. But we already have a test for that (see TckRecoveryTests#testCancelWhenParticipantIsUnavailable).

@@ -344,6 +346,31 @@ public void timeLimit() {
1, lraMetricService.getMetric(LRAMetricType.Compensated, lraId, LraResource.class.getName()));
}

/**
* Service A - Timeout 500 ms
* Service B - Type.MANDATORY``
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Service B - Type.MANDATORY``
* Service B - Type.MANDATORY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

return wae.getResponse();
} catch (InterruptedException e) {
LOGGER.log(Level.FINE, "Interrupted because time limit elapsed", e);
fail("Interrupted because time limit elapsed");
Copy link
Member

Choose a reason for hiding this comment

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

Will this work since this is a JAX-RS resource (i.e. not in the test)

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question, fail() (at least in junit4 master, today) seems to translate into an exception but I am not sure what a calling side receives if anything?

Copy link
Member

Choose a reason for hiding this comment

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

500 I believe since with exception it's failed JAX-RS invocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will return a response code instead.

// the next request should fail with a 412 code since the LRA should no longer be active
restPutInvocation(lraId, MANDATORY_LRA_RESOURCE_PATH, "");
} catch (WebApplicationException wae) {
return wae.getResponse();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return wae.getResponse();
return Response.status(wae.getResponse().getStatus()).build();

I tested this locally and the response returned in wae from restPutInvocation is actually closed in this method [1] which fails the underlying JAX-RS implementation at least in Narayana.

[1] https://github.com/eclipse/microprofile-lra/blob/master/tck/src/main/java/org/eclipse/microprofile/lra/tck/participant/api/LraResource.java#L327

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I pushed your recommended fix.

Copy link
Member

@rdebusscher rdebusscher left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

4 participants