issue122 context propagation tests #132

mmusgrov · 2019-04-07T21:32:43Z

This PR adds the TCK tests for the new context propagation semantics introduced in issue #122

xstefank · 2019-04-08T05:47:38Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/TckContextTests.java

+        String lra = invoke(NEW_LRA_PATH, PUT, null, 200, ContextTckResource.EndPhase.SUCCESS, 200);
+        invoke(REQUIRED_LRA_PATH, PUT, lra, 200, ContextTckResource.EndPhase.SUCCESS, 200);
+
+        // verify that the resource was not asked to complete


Suggested change

// verify that the resource was not asked to complete

// verify that the resource was asked to complete

I think this is a typo (based on the next assert)

xstefank · 2019-04-08T05:48:55Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/TckContextTests.java

+        // call a resource that begins and ends an LRA and coerces the resource to return ACCEPTED when asked to complete
+        String lra = invoke(REQUIRED_LRA_PATH, PUT, null, 200, ContextTckResource.EndPhase.ACCEPTED, 202);
+
+        // verfiy that the resource was asked to complete and is in the state Conpleting


Suggested change

// verfiy that the resource was asked to complete and is in the state Conpleting

// verfiy that the resource was asked to complete and is in the state Completing

xstefank · 2019-04-08T05:51:43Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/TckContextTests.java

+
+    @Test
+    public void testLeave() {
+        // call a resource that begins but does not ends an LRA


Suggested change

// call a resource that begins but does not ends an LRA

// call a resource that begins but does not end an LRA

xstefank · 2019-04-08T05:57:15Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/TckContextTests.java

+
+
+        assertEquals(testName.getMethodName() + ": Resource left but was still asked to complete",
+                Integer.toString(0), completions);


This is just personal preference - comparing integers by string equals seems unnecessary. Maybe we can add invokeMetric() method that would call invoke(...) in the same way and return parsed integer?

To keep the code simple I just changed it to "0" (instead of Integer.toString(0)) etc

I would still prefer it to be actual integer, but ok

xstefank · 2019-04-08T05:59:43Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/TckContextTests.java

+        replayEndPhase(TCK_CONTEXT_RESOURCE_PATH);
+
+        // the implementation should have called status which will have returned 500
+        String count1 = invoke(METRIC_PATH + "/" + Status.class.getName(), GET, lra);


Suggested change

String count1 = invoke(METRIC_PATH + "/" + Status.class.getName(), GET, lra);

String statusCount = invoke(METRIC_PATH + "/" + Status.class.getName(), GET, lra);

I simplified the code by declaring a variable called String count at the beginning of the method.

xstefank · 2019-04-08T06:00:29Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/TckContextTests.java

+        assertEquals(testName.getMethodName() + " resource status should have been called", "1", count1);
+
+        // the implementation should not call forget until it knows the particpant status
+        String count2 = invoke(METRIC_PATH + "/" + Forget.class.getName(), GET, lra);


Suggested change

String count2 = invoke(METRIC_PATH + "/" + Forget.class.getName(), GET, lra);

String forgetCount = invoke(METRIC_PATH + "/" + Forget.class.getName(), GET, lra);

xstefank · 2019-04-08T06:05:17Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/TckContextTests.java

+        assertEquals(testName.getMethodName() + " resource forget should not have been called", "0", count2);
+
+        // clear the fault
+        invoke(STATUS_PATH, PUT, lra, 200, ContextTckResource.EndPhase.FAILED, 200);


I would suggest using different JAX-RS resource method name for clearing/changing the status that is being returned from the resource to prevent confusion when next asserts check how many times @Status method has been called. I can see that this is PUT invocation, but I find this easy to miss :)

There should be no confusion since the semantics of PUT and GET are well known in REST based systems:

GET is for reading a resource representation

PUT is for updating the resource representation

@mmusgrov sure. I meant that it is easy to miss that this is actually a PUT invoke when it's placed on STATUS_PATH so I really just wanted to rename it to something like CLEAR_PATH or similar. But if you don't want to do this it's not big deal.

But then it would not be obvious that the same resource is being accessed.

I will add a commit that adds more documentation to clarify how the resource and tests interact ...

@xstefank I added more comments

xstefank · 2019-04-08T06:16:35Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/TckTestBase.java

+    }
+
+
+    void replayEndPhase(String resource) {


the parameter is not being used. Is the schema of recovery endpoint something we should define under recovery schematics? #116

Exactly, there should be an option to recover for a singe resource - that's why I added the parameter. But we don't need a schema, the resource to recover can be inferred from the location of the RECOVER annotation if we can get some agreement on that.

@mmusgrov will we use the option to recover a single resource in the TCK? if so, we should correctly implement the replayEndPhase then so that it can be used as such. Otherwise, remove the parameter.

This feature is still up for discussion. With my proposal on issue #116 it is per resource (but could be for all resources if required). BTW you said that you do not need the recovery so that issue will probably be moved out to the interoperability section of milestone 1.x. When/if that decision is made I will rewrite the replayEndPhase functionality to use a blocking wait for the implementation to get around to retrying the end phase.

The TCK already uses a proprietary technique and my plan is to leave this functionality as is until we decide what to do with #116 and like I said that will probably end up being an indefinite blocking call - I don't want to implement something in this PR and then find that I have rewrite it again depending on the outcome of the discussion.

@rdebusscher I know we discussed in the hangout that I would propose a PR for the recovery semantics and we would use that as a basis for discussion. But since I am the only person that thinks the RECOVERY annotation is useful I doubt it will make it into the spec (and we will need to fall back to something like "... after a crash the implementation must trigger recover at its earliest convenience ..."

xstefank · 2019-04-08T06:22:27Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/TckTests.java

-        // the value returned via the header and body should be equal
-
-        assertNotNull("JAX-RS response to PUT request should have returned the header " + LRA.LRA_HTTP_HEADER
+        // LRAs started programatically should not be available to the caller in the context header


this may be a little misleading to readers as with removal of LRA client there is no possibility to start LRA programatically. the "startViaApi" method uses LRAClientOps which still starts LRA by annotation.

xstefank · 2019-04-08T07:12:03Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/participant/api/ContextTckResource.java

+                LOGGER.warning("Invalid test data: endPhase ("
+                        + endPhase
+                        + ") or endPhaseStatus ("
+                        + endPhaseStatus + ")");


I would suggest to throw runtime exception here. This code will be called by TCK tests so this should really help TCK developer to find out that the wrong enum value has been used.

xstefank

looks good, thanks

rdebusscher

Few questions/remarks and a few improvements/corrections to make in my opinion.

tck/src/main/java/org/eclipse/microprofile/lra/tck/TckTestBase.java

rdebusscher · 2019-04-15T17:21:12Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/TckTestBase.java

+    }
+
+
+    void replayEndPhase(String resource) {


@mmusgrov will we use the option to recover a single resource in the TCK? if so, we should correctly implement the replayEndPhase then so that it can be used as such. Otherwise, remove the parameter.

rdebusscher · 2019-04-15T17:36:56Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/participant/api/ContextTckResource.java

+        void increment(String metric) {
+            if (metrics.containsKey(metric)) {
+                metrics.get(metric).incrementAndGet();
+            }


Should we be here on the safe side and throw an Exception if not found? (since we also return value -1 from the get() when not found?)

The test should fail if the the test passes in an invalid metric but I can add more code. Would you prefer an exception (which would eventually get mapped to a jax-rs status code) and I can update the test to assert that the invocation returns status OK

rdebusscher · 2019-04-15T17:43:12Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/participant/api/ContextTckResource.java

+
+    private EndPhase endPhase = EndPhase.SUCCESS;
+    // control which status to return from participant callbacks
+    private Response.Status endPhaseStatus = Response.Status.OK;


Is it ok to have only 1 participantStatus but keep a list of all LRAids (see metrics property on line 123)

There is only ever one LRA in play for each test. The reason I maintain a collection of LRAids is:

/**

A class to hold all of the metrics gathered in the context of a single LRA.
* We need stats per LRA since a misbehaving test may interfere with subsequent tests
*/

To further clarify why this could be a problem: a misbehaving test may leave an LRA in need of recovery which means that the compensate/complete call will continue to be called when subsequent tests run - ie it is not possible to fully tear down a failing test.

I have just pushed an update that includes that extra explanatory text.

rdebusscher · 2019-04-15T17:44:12Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/participant/api/ContextTckResource.java

+        endPhase = EndPhase.SUCCESS;
+        endPhaseStatus = Response.Status.OK;
+
+        if (metrics.containsKey(lraId)) {


We call /reset also with lraId = null (see TckContextTests:72 - @before - reset everything) -> so when null, we should clear out metric completely.

tck/src/main/java/org/eclipse/microprofile/lra/tck/participant/api/ContextTckResource.java

Signed-off-by: Michael Musgrove <[email protected]>

mmusgrov force-pushed the issue122 branch from cabb02e to 395cf21 Compare April 7, 2019 21:39

xstefank reviewed Apr 8, 2019

View reviewed changes

mmusgrov requested a review from rdebusscher April 8, 2019 12:34

mmusgrov force-pushed the issue122 branch from 65fb494 to b078183 Compare April 8, 2019 14:53

xstefank approved these changes Apr 8, 2019

View reviewed changes

mmusgrov mentioned this pull request Apr 13, 2019

Requirement of @LRA on @Leave annotated method: Clarify #122

Closed

rdebusscher suggested changes Apr 15, 2019

View reviewed changes

mmusgrov force-pushed the issue122 branch from 119037b to 7c559e0 Compare April 16, 2019 11:37

rdebusscher self-requested a review April 18, 2019 09:05

rdebusscher approved these changes Apr 18, 2019

View reviewed changes

issue122 context propagation tests

5773051

Signed-off-by: Michael Musgrove <[email protected]>

mmusgrov force-pushed the issue122 branch from a97db57 to 5773051 Compare April 18, 2019 09:53

mmusgrov merged commit 2b09048 into eclipse:master Apr 18, 2019

mmusgrov mentioned this pull request Apr 23, 2019

JBTM-3087 Ensure the LRA implementation is up to date (including new … jbosstm/narayana#1443

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue122 context propagation tests #132

issue122 context propagation tests #132

mmusgrov commented Apr 7, 2019

xstefank Apr 8, 2019

xstefank Apr 8, 2019

xstefank Apr 8, 2019

xstefank Apr 8, 2019

mmusgrov Apr 8, 2019

xstefank Apr 8, 2019

xstefank Apr 8, 2019

mmusgrov Apr 8, 2019

xstefank Apr 8, 2019

xstefank Apr 8, 2019

mmusgrov Apr 8, 2019

xstefank Apr 8, 2019

mmusgrov Apr 8, 2019

mmusgrov Apr 8, 2019

mmusgrov Apr 8, 2019

xstefank Apr 8, 2019

mmusgrov Apr 8, 2019

rdebusscher Apr 15, 2019

mmusgrov Apr 16, 2019

mmusgrov Apr 16, 2019

xstefank Apr 8, 2019

xstefank Apr 8, 2019

xstefank left a comment

rdebusscher left a comment

rdebusscher Apr 15, 2019

rdebusscher Apr 15, 2019

mmusgrov Apr 16, 2019 •

edited

Loading

rdebusscher Apr 15, 2019

mmusgrov Apr 16, 2019

mmusgrov Apr 16, 2019

mmusgrov Apr 16, 2019

rdebusscher Apr 15, 2019

mmusgrov Apr 16, 2019

	// verify that the resource was not asked to complete
	// verify that the resource was asked to complete

	// verfiy that the resource was asked to complete and is in the state Conpleting
	// verfiy that the resource was asked to complete and is in the state Completing

	// call a resource that begins but does not ends an LRA
	// call a resource that begins but does not end an LRA



		assertEquals(testName.getMethodName() + ": Resource left but was still asked to complete",
		Integer.toString(0), completions);

	String count1 = invoke(METRIC_PATH + "/" + Status.class.getName(), GET, lra);
	String statusCount = invoke(METRIC_PATH + "/" + Status.class.getName(), GET, lra);

	String count2 = invoke(METRIC_PATH + "/" + Forget.class.getName(), GET, lra);
	String forgetCount = invoke(METRIC_PATH + "/" + Forget.class.getName(), GET, lra);

issue122 context propagation tests #132

issue122 context propagation tests #132

Conversation

mmusgrov commented Apr 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xstefank left a comment

Choose a reason for hiding this comment

rdebusscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmusgrov Apr 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmusgrov Apr 16, 2019 •

edited

Loading