LRA TCK refactoring while adding Arquillian as integration point for the test #87

ochaloup · 2019-02-04T13:53:16Z

fixes #94

Refactoring of the TCK tests which provides Arquillian to base the integration on.
This adds Arquillian handling, changing few names and adding MicroProfile Config to the play.

When agreed on this approach and this is about to be merged I would like to continue with some more refactoring of the TCK codebase. There are still some comments and tests which needs to be clarified. This mostly migrates tests to be run with Arquillian but it does not do any changes in the logic of the tests.

The concept of the Arquillian integration is based on other spec (https://github.com/eclipse/microprofile-fault-tolerance/blob/master/tck/running_the_tck.asciidoc, https://github.com/eclipse/microprofile-health/blob/master/tck/running_the_tck.asciidoc).
The changes for Narayana LRA client are at: https://github.com/ochaloup/narayana/tree/lra-tck-arquillian-rework

As MicroProfile Config is used the timeout is configurable with changes provided here. The #60 are about to be covered by this PR.

Can you please take a look how you feel about this?: @mmusgrov @rdebusscher @xstefank

xstefank

+100 for adding javadoc explaining test scenarios

xstefank · 2019-02-05T16:07:42Z

tck/running_the_tck.asciidoc

+  time that the tests waits for the coordinator start-up
+`lra.tck.coordinator.hostname`::
+  hostname where coordinator is available at for TCK suite can run the LRA status from the coordinator
+`lra.tck.coordinator.port`::


why this and hostname cannot be directly configured by client configuration? e. g. lra.http.url

correct, it should be possible and it should not be ambiguous as currently is. I will check how is that possible.

xstefank · 2019-02-05T16:11:19Z

tck/running_the_tck.asciidoc

+
+=== MicroProfile Config being available
+
+The testsuite uses uses configuration while expecting MicroProfile Config is available. The maven coordinates


xstefank · 2019-02-05T16:12:59Z

tck/running_the_tck.asciidoc


-    curl -XPUT http://localhost:8080/tck/all?verbose=false | jq
+The `arquillian-*.xml` has to define a container that will be starated and managed by Arquillian lifecycle


xstefank · 2019-02-05T16:47:07Z

tck/running_the_tck.asciidoc

+
+=== ResourceProvider of base URL endpoint
+
+As TCK deployment provides LRA annotated JAX-RS classes which exposes REST endpoints. The TCK suite needs to call them.


it seems that dot after "endpoints" seems unintended

xstefank · 2019-02-05T16:49:51Z

tck/running_the_tck.asciidoc

+
+    @Override
+    public Object lookup(ArquillianResource arquillianResource, Annotation... annotations) {
+        return new URL("http://localhost:8080");


could this be done also by MP config property? To stay consistent. I guess this is because Arquillian initializes it's injections before CDI does.

sure, this could be done with MP config propery. My intention was that this forces the implementer to really provide the information. Using MP config property is no issue. Originally I have defined it so. I will change it.

xstefank · 2019-02-05T17:55:14Z

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

    }

    /*
     * Participants can pass data during enlistment and this data will be returned during
     * the complete/compensate callbacks
     */
+    // TODO: is this test to be run?


there is no point in keeping it if it won't be run

cleaning tests are planned as the next step. I need to discuss the intent with the author.

Ditto (I forget why I disabled some tests but the ones you have marked @ignore will need adding to issue #92 )

xstefank · 2019-02-05T18:00:11Z

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

-            if (readEntity) {
-                return response.readEntity(String.class);
-            }
+            assertEquals("Not expected status at call ' " + resourcePath.getUri() + "'",


redundant space after '

xstefank · 2019-02-05T18:17:49Z

tck/src/main/java/org/eclipse/microprofile/lra/tck/participant/activity/ActivityStorage.java

+    public Activity getActivityAndAssertExistence(String lraId, UriInfo jaxrsContext) {
+        if(!activities.containsKey(lraId)) {
+           String errorMessage = String.format("Activity store does not contain LRA id '%s' "
+                   + "while it was invoked method at '%s'", lraId, jaxrsContext.getPath());


invoked by the method at?

xstefank · 2019-02-05T18:26:07Z

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

             * attribute update the timeLimit to 300 sleep for 200 return from the method so the LRA will
             * have been running for 200 ms so it should not be cancelled
             */
            lraClient.renewTimeLimit(lraToURL(lraId, "Invalid LRA id"), 300, ChronoUnit.MILLIS);
            // sleep for 200000 micro seconds (should be longer than specified in the timeLimit annotation attribute)
            Thread.sleep(200);
        } catch (InterruptedException e) {
-            e.printStackTrace();
+            LOGGER.log(Level.FINE, "Interrupted because the revewed time limit elapsed", e);


xstefank · 2019-02-05T18:30:42Z

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

+        if(value < 0){
+            throw new IllegalArgumentException("value to adjust can't be negative");
+        }
+        return (int) Math.ceil(value * factor);


you can reuse long variant I think

ochaloup · 2019-02-05T21:38:10Z

@xstefank

+100 for adding javadoc explaining test scenarios

I see your point and I'm really for adding the javadoc comment to describe what is the purpose of the testcases. Just bear in mind that the current change is meant to re-factor from old http execution style to junit+arquillian. Comments were not part of the codebase before and they are not either for now.

I didn't implement the test cases. If I add the comments I need to study the purpose and reason about validity of the test cases. Please, believe me, I can promise I will add javadoc description as the next step.
Just for now, I would really like this re-factoring being committed first.

mmusgrov

It's looking great, I only had a few comments.

Also can you indicate somewhere how to debug the tests (either in the docs or on this PR).

mmusgrov · 2019-02-06T09:20:51Z

tck/running_the_tck.asciidoc

+  The URL where the TCK suite deployment is exposed at. The TCK suite will construct path based on this URL.
+  The default base URL is `http://localhost:8180`.
+`lra.tck.timeout.factor`::
+  Timeout factor of default value `1.0`. When set bigger than `1.0` then timeout value will be bigger and waiting time are longer.


You mean it is a multiplier for the property lra.tck.coordinator.waiting . If so, why not just let the user tune the value to what is appropriate for his test environment.

the lra.tck.timeout.factor is used to influence any timeout (any waiting time) in the TCK suite I enhanced the description to be more comprehensible.
I removed the waiting time completely for now. The TCK implementor can handle the waiting time for coordinator startup on his own.

mmusgrov · 2019-02-06T11:11:06Z

tck/running_the_tck.asciidoc

+  When set-up lower then the timeouts will be shorter.
+`lra.tck.coordinator.waiting`::
+  Time that the tests waits for the LRA recovery starts-up.
+`lra.http.host`, `lra.http.port`::


Could we use an authority strings for endpoints instead (https://tools.ietf.org/html/rfc3986#section-3.2) or maybe just say it is a URI which gives us maximum flexibility.

this parameters will be removed completely as discussed at the meeting the coordinator should not be expected by TCKs

mmusgrov · 2019-02-06T11:17:08Z

tck/running_the_tck.asciidoc

-To enable the tests in your implementation of this specification you need to add the
-following dependency to your build:
+* `pom.xml` dependencies are set-up
+* MicroProfile Config is providedby runtime for the LRA TCK suite could be run


This doesn't read very well. Instead I would just say:

MicroProfile Config

Or:

the LRA TCK suite requires MicroProfile Config

mmusgrov · 2019-02-06T11:19:09Z

tck/running_the_tck.asciidoc

+* a default Arquillian container is configured in `arquillian.xml` (tests manually deploy with use of `@ArquillianResource Deployer`)
+* implementation provides one implementation of `org.eclipse.microprofile.lra.client.LRAClient` as it's injected by TCK suite
+* implementation has to provide one implementation of `org.eclipse.microprofile.lra.tck.spi.ManagementSPI`. This is an interface
+  with definition of util methods used by the TCK suite for its run.


I guess you will remove this requirement if we end up removing the ManagementSPI interface.

yes, correct

mmusgrov · 2019-02-06T11:24:46Z

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

+     */
+    @Inject @ConfigProperty(name = LRAClient.LRA_RECOVERY_PATH_KEY, defaultValue = "lra-recovery-coordinator")
+    private int recoveryPath;
+


PR #68 is removing all of these properties

ok, I see. There will be just lra.http.recovery.url. When the PR#68 is merged I can change this for url too.

mmusgrov · 2019-02-06T11:27:52Z

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

-        lraClient.close();
-        msClient.close();
-        rcClient.close();
+        if(tckSuiteClient != null) tckSuiteClient.close();


Do we use checkstyle in the TCK?

yes, the checkstyle is used and it's active. Do you mean that you don't like this without use of curly braces?

if(tckSuiteClient != null) { tckSuiteClient.close(); }

If you are about to add this rule to the checkstyle I can change the code here.

I think that all projects should be using the same set of rules but I see that ours is different since we have commented out NeedBraces:

!--module name="NeedBraces"

I would recommend that that rule is re-enabled.

I will check to see where else we are different from other projects and if we are I will raise a separate issue for it (hopefully this is the only one).

mmusgrov · 2019-02-06T11:31:44Z

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

    @Test
-    private String isCompensatedLRA() throws WebApplicationException {
-        URL lra = lraClient.startLRA(null, "SpecTest#isCompensatedLRA", LRA_TIMEOUT_MILLIS, ChronoUnit.MILLIS);
+    @Ignore


I forget why I disabled some tests but the ones you have marked @ignore will need adding to issue #92

+1

yes, the tests marked as @Ignore are not run in current TCK. Even they were not marked as Ignore they were rejected from running by not being defined in the list of permitted tests.
Are you fine that this step of checking what are purposes of tests will be run as separate follow-up task after this PR will be merged and when SmallRye client implementation is merged to repo (for being possible to run the TCK tests - e.g. those ignored ones - with some existing implementation)?

mmusgrov · 2019-02-06T11:38:14Z

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

    @Test
-    private String isCompletedLRA() throws WebApplicationException {
-        URL lra = lraClient.startLRA(null, "SpecTest#isCompletedLRA", LRA_TIMEOUT_MILLIS, ChronoUnit.MILLIS);
+    @Ignore


Ditto (I forget why I disabled some tests but the ones you have marked @ignore will need adding to issue #92 )

mmusgrov · 2019-02-06T11:38:24Z

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

    @Test
-    private String joinLRAViaBody() throws WebApplicationException {
+    @Ignore


Ditto (I forget why I disabled some tests but the ones you have marked @ignore will need adding to issue #92 )

mmusgrov · 2019-02-06T11:40:18Z

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

    }

    /*
     * Participants can pass data during enlistment and this data will be returned during
     * the complete/compensate callbacks
     */
+    // TODO: is this test to be run?


Ditto (I forget why I disabled some tests but the ones you have marked @ignore will need adding to issue #92 )

ochaloup · 2019-02-06T21:38:40Z

@mmusgrov

It's looking great, I only had a few comments.

Thanks. I fixed the point you have. I would like to remove the ManagementSPI in the next step when each test case should be examined. There will be point to add all of them under issue #92.

Also can you indicate somewhere how to debug the tests (either in the docs or on this PR).

I added a short section on debugging but the way depends on the runtime where the TCK suite is deployed.

mmusgrov · 2019-02-08T13:41:02Z

@ochaloup Have you tried testing the new TCK against any LRA implementations yet?

ochaloup · 2019-02-08T14:12:11Z

@mmusgrov as mentioned in the description I was working with Narayana implementation https://github.com/ochaloup/narayana/tree/lra-tck-arquillian-rework. But I think it does not follow all the changes happened during last week (or two). The LRA client branch needs changes to be aligned and working.

mmusgrov · 2019-02-08T14:16:36Z

Can we control which snapshot build get's pulled in?

mmusgrov

My only remaining issue was if you could uncomment the rule module name="NeedBraces"
If other stuff breaks as a result of this and you don't want to fix them then just ignore this comment and I wil raise a separate issue.

ochaloup · 2019-02-08T14:38:39Z

@mmusgrov for Narayana I don't know other way then define the snapshot in pom.xml. Or we can be creating some tags in this repo and get those particular ones built in CI.
But I think the best would be to just fix the Narayana LRA when some change happens in the spec.

…o be on separate line Signed-off-by: Ondra Chaloupka <[email protected]>

… being passed as null Signed-off-by: Ondra Chaloupka <[email protected]>

ochaloup · 2019-02-08T15:42:52Z

@mmusgrov I updated the checkstyle as you mentioned and fixed the merge trouble caused by the changes in spec

rdebusscher · 2019-02-11T07:00:04Z

@mmusgrov as mentioned in the description I was working with Narayana implementation https://github.com/ochaloup/narayana/tree/lra-tck-arquillian-rework. But I think it does not follow all the changes happened during last week (or two). The LRA client branch needs changes to be aligned and working.

@ochaloup

Does the TCK run with implementation and coordinator? It should not be all green and use our latest decisions, but we need to make sure that it runs. Finetuning (on the tck side and the implementation) can be done afterward.

Signed-off-by: Ondra Chaloupka <[email protected]> Applying changes based on issue59

ochaloup · 2019-02-11T07:19:19Z

I just fixed typos that was mentioned by @xstefank in his comments and I forgot to address them. Otherwise no other changes with this force push.

ochaloup · 2019-02-11T07:26:42Z

@rdebusscher the idea is that the implementator of the TCK will ensure:

to start and running the coordinator (or the coordination endpoints defined by the spec)
configure the Arquillian to run with his implementation for Arquillian can start a container and deploy there tests
to implement the prescribed API/SPI (currently it's ManagementSPI)

So the Narayana TCK runner is about to start the Narayana LRA coordinator and to configure Arquillian to be capable to deploy tests.

Is that what you meant?

rdebusscher · 2019-02-11T07:57:00Z

@ochaloup

Is that what you meant?

Yes and no :)

We just need to make sure this updated code actually run with 'something' already (since the TCK is part of something bigger and cannot run on his own. You need those 3 points you mentioned)
But if this runs with the Narayana TCK, then it is fine.

I did assume that already. That is why I approved this PR.

ochaloup force-pushed the tck-refactoring-arquillian branch from bda9ccc to b986f0f Compare February 4, 2019 13:55

ochaloup mentioned this pull request Feb 4, 2019

TCK tests time limits should be configurable #60

Closed

ochaloup force-pushed the tck-refactoring-arquillian branch from b986f0f to b455832 Compare February 5, 2019 11:58

xstefank requested changes Feb 5, 2019

View reviewed changes

ochaloup force-pushed the tck-refactoring-arquillian branch 2 times, most recently from f90705e to e653b45 Compare February 5, 2019 23:00

mmusgrov requested changes Feb 6, 2019

View reviewed changes

ochaloup force-pushed the tck-refactoring-arquillian branch 2 times, most recently from 72f5d45 to 548616b Compare February 6, 2019 21:36

ochaloup force-pushed the tck-refactoring-arquillian branch from 548616b to cad5d01 Compare February 6, 2019 21:46

mmusgrov approved these changes Feb 8, 2019

View reviewed changes

ochaloup added 2 commits February 8, 2019 16:27

[typo fix] for the javadoc warning during compilation - param needs t…

378715e

…o be on separate line Signed-off-by: Ondra Chaloupka <[email protected]>

Overriding constructor of InvalidLRAIdException for not forcing cause…

4b99e43

… being passed as null Signed-off-by: Ondra Chaloupka <[email protected]>

ochaloup force-pushed the tck-refactoring-arquillian branch from cad5d01 to 62a3cb6 Compare February 8, 2019 15:41

mmusgrov requested a review from rdebusscher February 8, 2019 15:52

rdebusscher approved these changes Feb 11, 2019

View reviewed changes

issue microprofile#94: TCK refactoring for Arquillian

7172f13

Signed-off-by: Ondra Chaloupka <[email protected]> Applying changes based on issue59

ochaloup force-pushed the tck-refactoring-arquillian branch from 62a3cb6 to 7172f13 Compare February 11, 2019 07:18

xstefank approved these changes Feb 11, 2019

View reviewed changes

mmusgrov merged commit 94b7af4 into microprofile:master Feb 11, 2019

This was referenced Feb 14, 2019

issue #94: fixes on arquillian refactoring #96

Merged

[JBTM-3109] LRA TCK Arquillian rework jbosstm/narayana#1408

Merged

Issue94 tck arq refactoring next fixes #98

Merged


		=== MicroProfile Config being available

		The testsuite uses uses configuration while expecting MicroProfile Config is available. The maven coordinates


		curl -XPUT http://localhost:8080/tck/all?verbose=false \| jq
		The `arquillian-*.xml` has to define a container that will be starated and managed by Arquillian lifecycle


		=== ResourceProvider of base URL endpoint

		As TCK deployment provides LRA annotated JAX-RS classes which exposes REST endpoints. The TCK suite needs to call them.

LRA TCK refactoring while adding Arquillian as integration point for the test #87

LRA TCK refactoring while adding Arquillian as integration point for the test #87

Conversation

ochaloup commented Feb 4, 2019 • edited Loading

xstefank left a 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

ochaloup commented Feb 5, 2019 • edited Loading

mmusgrov left a 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

ochaloup Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

mmusgrov Feb 8, 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

ochaloup commented Feb 6, 2019 • edited Loading

mmusgrov commented Feb 8, 2019

ochaloup commented Feb 8, 2019 • edited Loading

mmusgrov commented Feb 8, 2019

mmusgrov left a comment

Choose a reason for hiding this comment

ochaloup commented Feb 8, 2019

ochaloup commented Feb 8, 2019

rdebusscher commented Feb 11, 2019

ochaloup commented Feb 11, 2019

ochaloup commented Feb 11, 2019

rdebusscher commented Feb 11, 2019 • edited Loading

ochaloup commented Feb 4, 2019 •

edited

Loading

ochaloup commented Feb 5, 2019 •

edited

Loading

ochaloup Feb 6, 2019 •

edited

Loading

mmusgrov Feb 8, 2019 •

edited

Loading

ochaloup commented Feb 6, 2019 •

edited

Loading

ochaloup commented Feb 8, 2019 •

edited

Loading

rdebusscher commented Feb 11, 2019 •

edited

Loading