Using alternate dispatcher and delegator for integration tests

[Mathieu Lirzin]

Hello,

I am trying to simplify the way integration tests are currently run.
While looking at ‘org.apache.ofbiz.testtools.ModelTestSuite’ I have
found that it is possible to define alternate dispatcher and delegator
for specific tests.

This feature is not used anywhere in the framework and introduces a ton
of complexity since we can't assume that there is only one dispatcher
and delegator shared by all the tests cases.

As a consequence I would like to know if we could agree on removing this
unused feature?

Thanks.

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[taher]

An example could shed some light here perhaps? What do you want to
remove from where? How is it complex?
On Sun, Oct 21, 2018 at 5:17 PM Mathieu Lirzin
<[hidden email]> wrote:

>
> Hello,
>
> I am trying to simplify the way integration tests are currently run.
> While looking at ‘org.apache.ofbiz.testtools.ModelTestSuite’ I have
> found that it is possible to define alternate dispatcher and delegator
> for specific tests.
>
> This feature is not used anywhere in the framework and introduces a ton
> of complexity since we can't assume that there is only one dispatcher
> and delegator shared by all the tests cases.
>
> As a consequence I would like to know if we could agree on removing this
> unused feature?
>
> Thanks.
>
> --
> Mathieu Lirzin
> GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Mathieu Lirzin]

Hello Taher,

Taher Alkhateeb <[hidden email]> writes:

> An example could shed some light here perhaps?

What kind of examples would help you?

AIUI It would be for the people using this feature outside of the
framework to provide examples why we should keep it in.

> What do you want to remove from where?

Here is what I precisely want to remove.


From 66150cc98d2b7b84ee5aa4dee1e25f60556577e6 Mon Sep 17 00:00:00 2001
From: Mathieu Lirzin <[hidden email]>
Date: Sun, 21 Oct 2018 16:02:38 +0200
Subject: [PATCH] Disallow using alternate dispatcher and delegator for
 integration tests

---
 framework/testtools/dtd/test-suite.xsd        |  2 --
 .../ofbiz/testtools/ModelTestSuite.java       | 23 +++++--------------
 2 files changed, 6 insertions(+), 19 deletions(-)

diff --git framework/testtools/dtd/test-suite.xsd framework/testtools/dtd/test-suite.xsd
index ee1767aa2f..76cdf1a523 100644
--- framework/testtools/dtd/test-suite.xsd
+++ framework/testtools/dtd/test-suite.xsd
@@ -45,8 +45,6 @@ under the License.
     </xs:element>
     <xs:attributeGroup name="attlist.test-suite">
         <xs:attribute type="xs:string" name="suite-name" use="required"/>
-        <xs:attribute type="xs:string" name="delegator-name" default="test"/>
-        <xs:attribute type="xs:string" name="dispatcher-name" default="test-dispatcher"/>
     </xs:attributeGroup>
     <xs:element name="test-case">
         <xs:annotation><xs:documentation></xs:documentation></xs:annotation>
diff --git framework/testtools/src/main/java/org/apache/ofbiz/testtools/ModelTestSuite.java framework/testtools/src/main/java/org/apache/ofbiz/testtools/ModelTestSuite.java
index 5c166beb5d..e666e4451a 100644
--- framework/testtools/src/main/java/org/apache/ofbiz/testtools/ModelTestSuite.java
+++ framework/testtools/src/main/java/org/apache/ofbiz/testtools/ModelTestSuite.java
@@ -50,31 +50,20 @@ import junit.framework.TestSuite;
  * Use this class in a JUnit test runner to bootstrap the Test Suite runner.
  */
 public class ModelTestSuite {
-
     public static final String module = ModelTestSuite.class.getName();
+    public static final String DELEGATOR_NAME = "test";
+    public static final String DISPATCHER_NAME = "test-dispatcher";
 
     protected String suiteName;
-    protected String originalDelegatorName;
-    protected String originalDispatcherName;
-
     protected Delegator delegator;
     protected LocalDispatcher dispatcher;
-
-    protected List<Test> testList = new ArrayList<Test>();
+    protected List<Test> testList = new ArrayList<>();
 
     public ModelTestSuite(Element mainElement, String testCase) {
-        this.suiteName = mainElement.getAttribute("suite-name");
-
-        this.originalDelegatorName = mainElement.getAttribute("delegator-name");
-        if (UtilValidate.isEmpty(this.originalDelegatorName)) this.originalDelegatorName = "test";
-
-        this.originalDispatcherName = mainElement.getAttribute("dispatcher-name");
-        if (UtilValidate.isEmpty(this.originalDispatcherName)) this.originalDispatcherName = "test-dispatcher";
-
         String uniqueSuffix = "-" + RandomStringUtils.randomAlphanumeric(10);
-
-        this.delegator = DelegatorFactory.getDelegator(this.originalDelegatorName).makeTestDelegator(this.originalDelegatorName + uniqueSuffix);
-        this.dispatcher = ServiceContainer.getLocalDispatcher(originalDispatcherName + uniqueSuffix, delegator);
+        this.suiteName = mainElement.getAttribute("suite-name");
+        this.delegator = DelegatorFactory.getDelegator(DELEGATOR_NAME).makeTestDelegator(DELEGATOR_NAME + uniqueSuffix);
+        this.dispatcher = ServiceContainer.getLocalDispatcher(DISPATCHER_NAME+ uniqueSuffix, delegator);
 
         for (Element testCaseElement : UtilXml.childElementList(mainElement, UtilMisc.toSet("test-case", "test-group"))) {
             String caseName = testCaseElement.getAttribute("case-name");
--
2.19.1



> How is it complex?

Basically each time an option is provided it adds complexity.  In this
particular case, perhaps a comparaison between the two models of test
execution can help describing the added complexity:

Current model:
   1. Launch OFBiz
   2. Fetch all the tests from the components
   3. Keep track which delegator/dispatcher correspond to each test suite
   4. Run each test by setting its corresponding delegator/dispatcher

Model without option:
   1. Launch OFBiz with the test delegator/dispatcher
   2. Fetch all the tests from the components
   3. Run the tests.

Does it help?

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[taher]

Ahh, Now I understand what you mean by looking at your patch. I
recommend next time that you copy-paste into the email thread because
people who try to access this thread using the ML archives might not
see your attachments.

At a first glance, although this feature is not used in the current
OFBiz code base. it _might_ be used by some users who are running
certain tests against specific tenants for example. So although I
would probably lean towards removing it, I'd recommend making sure
that not many people depend on this feature. Perhaps you can check in
the user ML or wait for others to share their opinions.
On Sun, Oct 21, 2018 at 10:51 PM Mathieu Lirzin
<[hidden email]> wrote:

>
> Hello Taher,
>
> Taher Alkhateeb <[hidden email]> writes:
>
> > An example could shed some light here perhaps?
>
> What kind of examples would help you?
>
> AIUI It would be for the people using this feature outside of the
> framework to provide examples why we should keep it in.
>
> > What do you want to remove from where?
>
> Here is what I precisely want to remove.
>
>
> > How is it complex?
>
> Basically each time an option is provided it adds complexity.  In this
> particular case, perhaps a comparaison between the two models of test
> execution can help describing the added complexity:
>
> Current model:
>    1. Launch OFBiz
>    2. Fetch all the tests from the components
>    3. Keep track which delegator/dispatcher correspond to each test suite
>    4. Run each test by setting its corresponding delegator/dispatcher
>
> Model without option:
>    1. Launch OFBiz with the test delegator/dispatcher
>    2. Fetch all the tests from the components
>    3. Run the tests.
>
> Does it help?
>
> --
> Mathieu Lirzin
> GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Scott Gray-3]

The use of separate individual dispatchers/delegators per test suite was
introduced at the same time as the delegator rollback/reset code, and was
intended to allow for test suites to be run (and data to be reset) in
parallel.

The test framework never sees much love though so that never happened. It's
difficult to accomplish because many test suites rely on the same
underlying demo data.

I don't mind either way, just thought the history lesson might be helpful.

https://issues.apache.org/jira/browse/OFBIZ-2259
https://markmail.org/thread/zjghmk25bllthxme

Regards
Scott

On Sat, 27 Oct 2018, 22:59 Taher Alkhateeb, <[hidden email]>
wrote:

> Ahh, Now I understand what you mean by looking at your patch. I
> recommend next time that you copy-paste into the email thread because
> people who try to access this thread using the ML archives might not
> see your attachments.
>
> At a first glance, although this feature is not used in the current
> OFBiz code base. it _might_ be used by some users who are running
> certain tests against specific tenants for example. So although I
> would probably lean towards removing it, I'd recommend making sure
> that not many people depend on this feature. Perhaps you can check in
> the user ML or wait for others to share their opinions.
> On Sun, Oct 21, 2018 at 10:51 PM Mathieu Lirzin
> <[hidden email]> wrote:
> >
> > Hello Taher,
> >
> > Taher Alkhateeb <[hidden email]> writes:
> >
> > > An example could shed some light here perhaps?
> >
> > What kind of examples would help you?
> >
> > AIUI It would be for the people using this feature outside of the
> > framework to provide examples why we should keep it in.
> >
> > > What do you want to remove from where?
> >
> > Here is what I precisely want to remove.
> >
> >
> > > How is it complex?
> >
> > Basically each time an option is provided it adds complexity.  In this
> > particular case, perhaps a comparaison between the two models of test
> > execution can help describing the added complexity:
> >
> > Current model:
> >    1. Launch OFBiz
> >    2. Fetch all the tests from the components
> >    3. Keep track which delegator/dispatcher correspond to each test suite
> >    4. Run each test by setting its corresponding delegator/dispatcher
> >
> > Model without option:
> >    1. Launch OFBiz with the test delegator/dispatcher
> >    2. Fetch all the tests from the components
> >    3. Run the tests.
> >
> > Does it help?
> >
> > --
> > Mathieu Lirzin
> > GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37
>

[taher]

I keep forgetting this project carries a lot of history with it. Maybe
it's because I'm a relatively new. Thank you for sharing Scott, quite
informative.

If the main advantage is parallelization but that was not really used,
then maybe it's not super necessary. Maybe one way to think about this
is instead of making our tests faster, let's make them lighter. For
example we can switch out from integration tests to unit tests and do
away with the extra layer of resources being consumed.

Anyway, all options on the table, like Scott no firm opinion here.

On Mon, Oct 29, 2018 at 1:16 AM Scott Gray <[hidden email]> wrote:

>
> The use of separate individual dispatchers/delegators per test suite was
> introduced at the same time as the delegator rollback/reset code, and was
> intended to allow for test suites to be run (and data to be reset) in
> parallel.
>
> The test framework never sees much love though so that never happened. It's
> difficult to accomplish because many test suites rely on the same
> underlying demo data.
>
> I don't mind either way, just thought the history lesson might be helpful.
>
> https://issues.apache.org/jira/browse/OFBIZ-2259
> https://markmail.org/thread/zjghmk25bllthxme
>
> Regards
> Scott
>
> On Sat, 27 Oct 2018, 22:59 Taher Alkhateeb, <[hidden email]>
> wrote:
>
> > Ahh, Now I understand what you mean by looking at your patch. I
> > recommend next time that you copy-paste into the email thread because
> > people who try to access this thread using the ML archives might not
> > see your attachments.
> >
> > At a first glance, although this feature is not used in the current
> > OFBiz code base. it _might_ be used by some users who are running
> > certain tests against specific tenants for example. So although I
> > would probably lean towards removing it, I'd recommend making sure
> > that not many people depend on this feature. Perhaps you can check in
> > the user ML or wait for others to share their opinions.
> > On Sun, Oct 21, 2018 at 10:51 PM Mathieu Lirzin
> > <[hidden email]> wrote:
> > >
> > > Hello Taher,
> > >
> > > Taher Alkhateeb <[hidden email]> writes:
> > >
> > > > An example could shed some light here perhaps?
> > >
> > > What kind of examples would help you?
> > >
> > > AIUI It would be for the people using this feature outside of the
> > > framework to provide examples why we should keep it in.
> > >
> > > > What do you want to remove from where?
> > >
> > > Here is what I precisely want to remove.
> > >
> > >
> > > > How is it complex?
> > >
> > > Basically each time an option is provided it adds complexity.  In this
> > > particular case, perhaps a comparaison between the two models of test
> > > execution can help describing the added complexity:
> > >
> > > Current model:
> > >    1. Launch OFBiz
> > >    2. Fetch all the tests from the components
> > >    3. Keep track which delegator/dispatcher correspond to each test suite
> > >    4. Run each test by setting its corresponding delegator/dispatcher
> > >
> > > Model without option:
> > >    1. Launch OFBiz with the test delegator/dispatcher
> > >    2. Fetch all the tests from the components
> > >    3. Run the tests.
> > >
> > > Does it help?
> > >
> > > --
> > > Mathieu Lirzin
> > > GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37
> >

[Mathieu Lirzin]

Taher Alkhateeb <[hidden email]> writes:

> I keep forgetting this project carries a lot of history with it. Maybe
> it's because I'm a relatively new. Thank you for sharing Scott, quite
> informative.

Same here.

> If the main advantage is parallelization but that was not really used,
> then maybe it's not super necessary. Maybe one way to think about this
> is instead of making our tests faster, let's make them lighter. For
> example we can switch out from integration tests to unit tests and do
> away with the extra layer of resources being consumed.

An extra argument for prefering unit tests over integration tests is
that they are far more parallelizable since they do to not rely on
shared ressources.

In order to achieve that, more refactoring would be needed to make the
code easily unit testable, such as:

  - adding interfaces to decouple implementations
  - adding alternate model constructors which doesn't impose passing
    an XML element
  - removing abuse of inheritance and replace it with delegation
  - injecting dependencies instead of embedding them

So Strong +1!

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37