SME Review

[MaiHameed]

Provided by @aguibert

1. Why are we adding rxjava3 as a dependency? We should be using MP Reactive Streams instead of rxjava3.
2. In Order.java, you don't need to have getters and setters. Just use public fields instead
3. In KitchenService.java, the Thread.sleep calls will make the entire sample and Liberty itself look bad because things are slow. Please eliminate the sleeps, or at least reduce the sleepTime as much as possible. Also, if you are gong to keep the sleeps, they should be for a fixed amount of time instead of a random amount of time.
4. In KitchenService.java, we should not be doing Executors.newSingelThreadExecutor(), this is bad practice because it uses an unmanaged thread. Instead, we should use a ManagedExecutor from MP Context Propagation.
5. In KitchenServiceIT.java, the POLL_TIMEOUT variable is unused and can be removed.
6. In KitchenServiceIT.java, since the two tests are order dependent, they should be combined into a single @Test method instead of using the JUnit @Order annotation.
7. I'm not sure what this sentence is trying to say? What is a "mimmic test container"? "Second is the kitchen container, which uses the same container that you built instead of a mimic test container."

[ankitagrawa]

Why are we adding rxjava3 as a dependency? We should be using MP Reactive Streams instead of rxjava3.

SME review feedback ( Andrew Rousse) :

There's no support in Reactive Streams to bridge from non-reactive code to reactive code, aside from doing a blocking wait with ReactiveStreams.generate()

[ankitagrawa]

In Order.java, you don't need to have getters and setters. Just use public fields instead

I don't agree. As per encapsulation principle you should always use getters and setters or if I misunderstood.

[ankitagrawa]

In KitchenService.java, we should not be doing Executors.newSingelThreadExecutor(), this is bad practice because it uses an unmanaged thread. Instead, we should use a ManagedExecutor from MP Context Propagation.

SME review feedback ( Andrew Rousse)

[hutchig]

I had a few blips which I started recording here: https://ibm-cloud.slack.com/archives/C6RHLBQRK/p1585573156016000?thread_ts=1585239416.007300&cid=C6RHLBQRK but should have used this issue.

Running through https://draft-guides.mybluemix.net/guides/reactive-service-testing.html#try-what-youll-build currently fails. Output attached: output.txt ( ‘docker run hello-world’ runs fine ) . On the text, I was a bit unsure how to make the step “Ensure you are using Linux containers, not Windows.” concrete - so I did nothing - I run on OSX and expect what goes on inside docker to be controlled by the instructions I am about to follow. “mvn liberty:dev” from ~/…../finish/kitchen runs the two tests OK though.

Going to https://openliberty.io/guides/microshed-testing.html#bootstrapping-your-application-for-testing and typing “mvn liberty:dev” also fails with “No plugin found for prefix ‘liberty’ in the current project and in the plugin groups [org.apache.maven.plugins, org.codehaus.mojo] available from the repositories [local (/Users/hutchig/.m2/repository), central (https://repo.maven.apache.org/maven2)]”

The reason mvn liberty:dev fails I think is because just before it is says “Navigate to the start directory to begin”, perhaps this should be “Navigate to the start/kichen directory to begin”? (Or the pom.xml in start needs a tweek.) as mvn liberty:dev works fine in start/kitchen.

[MaiHameed]

Hello @hutchig, thanks for the review. Which Java version are you running?

In relation to the second point, that's for a different guide, here: https://github.com/OpenLiberty/guide-microshed-testing, I've opened an issue regarding that to address separately.

[hutchig]

(I was on vacation yesterday - so apologies for replying on slack) $ java --version openjdk 13.0.2 2020-01-14 OpenJDK Runtime Environment AdoptOpenJDK (build 13.0.2+8) OpenJDK 64-Bit Server VM AdoptOpenJDK (build 13.0.2+8, mixed mode, sharing)

[hutchig]

Ok so going to https://draft-guides.mybluemix.net/guides/reactive-service-testing.html#configuring-your-containers and doing mvn -pl models install works fine for me and also I see the text says "Navigate to the start/kitchen directory." at the end of the para prior to mvn liberty:dev - so there is a chance I just got my (many) tabs mixed up. I guess one just needs to be in a folder for a server to get liberty:dev success. So this guide is fine in this.

[MaiHameed]

App has been refactored based on feedback, PR here: https://github.com/OpenLiberty/draft-guide-reactive-service-testing/pull/14, closing this issue, further discussion can be on the new guide model.