Removing support alternate dispatcher and delegator for integration tests

[Mathieu Lirzin]

Hello,

We are considering removing the support for setting an alternate
dispatcher and delegator when running integration tests.  This mechanism
adds complexity to the way OFBiz handles those tests [1].  it isn't used
anywhere in the framework but maybe some users relies on it elsewhere.

If you are concerned it is time to step up and discuss the rationale of
its usage, and maybe consider alternatives.

Here is the precise change which is considered to be applied in OFBiz:

--8<---------------cut here---------------start------------->8---
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
--8<---------------cut here---------------end--------------->8---

Thanks.


[1] https://lists.apache.org/thread.html/638e88265971cfb47ee03b87c6705206d8369c8d1d94fe8c8decfb74@%3Cdev.ofbiz.apache.org%3E

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

[Mathieu Lirzin]

Hello,

Since nobody has step up to discuss the rationale of having a custom
delegator/dispatcher per test, I think it is reasonable to remove this
feature.

I am going to submit the patch on Jira for review.

Mathieu Lirzin <[hidden email]> writes:

> We are considering removing the support for setting an alternate
> dispatcher and delegator when running integration tests.  This mechanism
> adds complexity to the way OFBiz handles those tests [1].  it isn't used
> anywhere in the framework but maybe some users relies on it elsewhere.
>
> If you are concerned it is time to step up and discuss the rationale of
> its usage, and maybe consider alternatives.
>
> Here is the precise change which is considered to be applied in OFBiz:
>
> 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(-)

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

[Michael Brohl-3]

+1, Mathieu!

Thanks,

Michael


Am 19.12.18 um 12:31 schrieb Mathieu Lirzin:

> Hello,
>
> Since nobody has step up to discuss the rationale of having a custom
> delegator/dispatcher per test, I think it is reasonable to remove this
> feature.
>
> I am going to submit the patch on Jira for review.
>
> Mathieu Lirzin <[hidden email]> writes:
>
>> We are considering removing the support for setting an alternate
>> dispatcher and delegator when running integration tests.  This mechanism
>> adds complexity to the way OFBiz handles those tests [1].  it isn't used
>> anywhere in the framework but maybe some users relies on it elsewhere.
>>
>> If you are concerned it is time to step up and discuss the rationale of
>> its usage, and maybe consider alternatives.
>>
>> Here is the precise change which is considered to be applied in OFBiz:
>>
>>  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(-)