Runner simplification refactoring #265
Conversation
| @Reference | ||
| private JmsConnection jmsConnection; | ||
|
|
||
| public CollectDispatcher getCollectDispatcher(TimeoutWatch timeoutWatch, |
There was a problem hiding this comment.
Could we change the names of methods in this class?
Currently they have names of getters, but they are NOT getters.
| collectionResultsRouter.addChangeObserver(comparisonResultsRouter); | ||
| } | ||
|
|
||
| void start() throws JMSException { |
There was a problem hiding this comment.
Could we rename this method? start could be confusing as is used when working with threads...
Maybe process ?
|
|
||
| import com.cognifide.aet.runner.testsuitescope.TestSuiteScoped; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Let's remove whole JAVADOC if we can or write some description. Currently it duplicates class name.
| @@ -36,14 +32,14 @@ public synchronized void update() { | |||
| * @param acceptedSecondsDifference accepted difference in seconds | |||
| * @return true if last update was done more than acceptedSecondsDifference seconds from now | |||
| */ | |||
| public synchronized boolean isTimedOut(long acceptedSecondsDifference) { | |||
There was a problem hiding this comment.
Thank you for changes in visibility/scope :)
| @@ -13,35 +13,26 @@ | |||
| * or implied. See the License for the specific language governing permissions and limitations under | |||
| @@ -1,3 +1,11 @@ | |||
| package com.cognifide.aet.runner.processing; | |||
There was a problem hiding this comment.
We usually have license header at the top of our files. could we have it consistent here?
| @@ -69,10 +69,6 @@ | |||
| <groupId>org.apache.activemq</groupId> | |||
| <artifactId>activemq-osgi</artifactId> | |||
| </dependency> | |||
| <dependency> | |||
There was a problem hiding this comment.
Awesome! Thank you @Skejven !
Actually it looks like guice is referenced also in worker module, but is not used at all there (or am I wrong?).
So we could remove Guice from the worker pom and then also from top-level pom, isn't it?
Then we could remove guice from feature.xml file. That would b great, right?
(there will be only some transitive dependencies to guice from our functional tests that use Bobcat).
Could be done here or in another pull request if you prefer...
There was a problem hiding this comment.
Thank you @wiiitek for the review.
I will remove Guice dependency in the separate PR.
Description
Curren AET Runner flow is very complicated with unnecessary creation of threads and Guice DI.
Motivation and Context
Complicated AET Runner implementation makes it harder to understand it and improve. This is the first refactor in a series of changes that I'd like to apply to the AET Runner.
I've tried to introduce as few changes at is possible, there are areas to improve (starting with introducing Java 8 features).
Types of changes
Checklist:
I hereby agree to the terms of the AET Contributor License Agreement.