OFBiz › OFBiz - Dev

[jira] Created: (OFBIZ-3748) Remove test specific code in the GeneralDelegator

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

5 messages Options

Nicolas Malin (Jira)

[jira] Created: (OFBIZ-3748) Remove test specific code in the GeneralDelegator

Remove test specific code in the GeneralDelegator
-------------------------------------------------

Key: OFBIZ-3748
URL: https://issues.apache.org/jira/browse/OFBIZ-3748
Project: OFBiz
Issue Type: Bug
Components: framework
Affects Versions: SVN trunk
Reporter: Bob Morley
Attachments: OFBIZ-3748_TestGenericDelegator.patch

Adam -- This is the results from our conversation on consistent rolling back of unit testers. We talked about moving the logic that is in the GenericDelegator that is specific to testing into a sub-class. This patch is NOT meant to be merged at this time, I wanted to put it up for review before I continue down this path ...

Here are the key pieces:

- TestGenericDelegator - test version of a generic delegator that contains the ability to record the list of database operations and then programmatically roll them back in reverse order. This was from existing code in GenericDelegator.

- TestDelegatorFactoryImpl - a new service implementation of DelegatoryFactor that will construct instances of TestGenericDelegator.

Things to consider:

- Should rollback be on the Delegator interface? I sort of field it should not be there; but I left it there for now with GenericDelegator reporting an error if it is called.

- Since there are two implementations of the DelegatorFactory there needs to be a way to determine which one to use; the way I have done this in the past is through configuration. Usually something like ... service.org.ofbiz.entity.DelegatorFactory=org.ofbiz.entity.DelegatorFactoryImpl that could (for Ofbiz) be placed in the start.properties or test.properties file. However, looking at the factory unit tester it looks like each factory should be able to determine if it is applicable based on the incoming parameters. As a result (until more discussion) I have made a choice based on the delegator name -- I know this is clearly NOT the go forward method. But would like some suggestions here ... was considering a new attribute on the entityengine.xml delegator definition, but there should be some mechanism to be able to provide control over which implementation is used I would think ...

- I got an inkling that "base delegator name" may not be required anymore. This is because I no longer create the standard delegator and then "clone" to a test version. I simply instantiate the proper version right up front ... Moreover, I let the delegator / dispatcher names be as they are (not adding a random alpha-numeric suffix). Not sure about this, did not research further.

Go forward plan --

- If there are agreement on these changes and a resolution for things to consider point #2 above, I would then re-code my standalone rollback base class for unit tests to leverage this functionality. This would ensure we consistently rollback regardless of executing the test directly or through the test infrastructure.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Nicolas Malin (Jira)

[jira] Updated: (OFBIZ-3748) Remove test specific code in the GeneralDelegator

[ https://issues.apache.org/jira/browse/OFBIZ-3748?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Bob Morley updated OFBIZ-3748:
------------------------------

Attachment: OFBIZ-3748_TestGenericDelegator.patch

> Remove test specific code in the GeneralDelegator
> -------------------------------------------------
>
> Key: OFBIZ-3748
> URL: https://issues.apache.org/jira/browse/OFBIZ-3748
> Project: OFBiz
> Issue Type: Bug
> Components: framework
> Affects Versions: SVN trunk
> Reporter: Bob Morley
> Attachments: OFBIZ-3748_TestGenericDelegator.patch
>
>
> Adam -- This is the results from our conversation on consistent rolling back of unit testers. We talked about moving the logic that is in the GenericDelegator that is specific to testing into a sub-class. This patch is NOT meant to be merged at this time, I wanted to put it up for review before I continue down this path ...
> Here are the key pieces:
> - TestGenericDelegator - test version of a generic delegator that contains the ability to record the list of database operations and then programmatically roll them back in reverse order. This was from existing code in GenericDelegator.
> - TestDelegatorFactoryImpl - a new service implementation of DelegatoryFactor that will construct instances of TestGenericDelegator.
> Things to consider:
> - Should rollback be on the Delegator interface? I sort of field it should not be there; but I left it there for now with GenericDelegator reporting an error if it is called.
> - Since there are two implementations of the DelegatorFactory there needs to be a way to determine which one to use; the way I have done this in the past is through configuration. Usually something like ... service.org.ofbiz.entity.DelegatorFactory=org.ofbiz.entity.DelegatorFactoryImpl that could (for Ofbiz) be placed in the start.properties or test.properties file. However, looking at the factory unit tester it looks like each factory should be able to determine if it is applicable based on the incoming parameters. As a result (until more discussion) I have made a choice based on the delegator name -- I know this is clearly NOT the go forward method. But would like some suggestions here ... was considering a new attribute on the entityengine.xml delegator definition, but there should be some mechanism to be able to provide control over which implementation is used I would think ...
> - I got an inkling that "base delegator name" may not be required anymore. This is because I no longer create the standard delegator and then "clone" to a test version. I simply instantiate the proper version right up front ... Moreover, I let the delegator / dispatcher names be as they are (not adding a random alpha-numeric suffix). Not sure about this, did not research further.
> Go forward plan --
> - If there are agreement on these changes and a resolution for things to consider point #2 above, I would then re-code my standalone rollback base class for unit tests to leverage this functionality. This would ensure we consistently rollback regardless of executing the test directly or through the test infrastructure.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Nicolas Malin (Jira)

[jira] Updated: (OFBIZ-3748) Remove test specific code in the GenericDelegator

In reply to this post by Nicolas Malin (Jira)

> Remove test specific code in the GenericDelegator
> -------------------------------------------------
>
> Key: OFBIZ-3748
> URL: https://issues.apache.org/jira/browse/OFBIZ-3748
> Project: OFBiz
> Issue Type: Bug
> Components: framework
> Affects Versions: SVN trunk
> Reporter: Bob Morley
> Attachments: OFBIZ-3748_TestGenericDelegator.patch
>
>
> Adam -- This is the results from our conversation on consistent rolling back of unit testers. We talked about moving the logic that is in the GenericDelegator that is specific to testing into a sub-class. This patch is NOT meant to be merged at this time, I wanted to put it up for review before I continue down this path ...
> Here are the key pieces:
> - TestGenericDelegator - test version of a generic delegator that contains the ability to record the list of database operations and then programmatically roll them back in reverse order. This was from existing code in GenericDelegator.
> - TestDelegatorFactoryImpl - a new service implementation of DelegatoryFactor that will construct instances of TestGenericDelegator.
> Things to consider:
> - Should rollback be on the Delegator interface? I sort of field it should not be there; but I left it there for now with GenericDelegator reporting an error if it is called.
> - Since there are two implementations of the DelegatorFactory there needs to be a way to determine which one to use; the way I have done this in the past is through configuration. Usually something like ... service.org.ofbiz.entity.DelegatorFactory=org.ofbiz.entity.DelegatorFactoryImpl that could (for Ofbiz) be placed in the start.properties or test.properties file. However, looking at the factory unit tester it looks like each factory should be able to determine if it is applicable based on the incoming parameters. As a result (until more discussion) I have made a choice based on the delegator name -- I know this is clearly NOT the go forward method. But would like some suggestions here ... was considering a new attribute on the entityengine.xml delegator definition, but there should be some mechanism to be able to provide control over which implementation is used I would think ...
> - I got an inkling that "base delegator name" may not be required anymore. This is because I no longer create the standard delegator and then "clone" to a test version. I simply instantiate the proper version right up front ... Moreover, I let the delegator / dispatcher names be as they are (not adding a random alpha-numeric suffix). Not sure about this, did not research further.
> Go forward plan --
> - If there are agreement on these changes and a resolution for things to consider point #2 above, I would then re-code my standalone rollback base class for unit tests to leverage this functionality. This would ensure we consistently rollback regardless of executing the test directly or through the test infrastructure.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Nicolas Malin (Jira)

[jira] Commented: (OFBIZ-3748) Remove test specific code in the GenericDelegator

In reply to this post by Nicolas Malin (Jira)

[ https://issues.apache.org/jira/browse/OFBIZ-3748?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12863559#action_12863559 ]

Scott Gray commented on OFBIZ-3748:
-----------------------------------

Is it correct to have two separate Factory implementations? I thought it was the job of the factory to decide which delegator implementation to provide?

About the "base delegator name", the question you aren't asking yourself is why was the delegator being cloned in the first place? I could have (and originally did) just turned on the test mode in the base delegator. Here's the original jira for reference: https://issues.apache.org/jira/browse/OFBIZ-2259
Although I would argue that it's impossible to run multiple test suites in parallel on the same database.

I like the approach you've taken, but let's be clear, all we're really achieving here is a little cleaner code by moving some methods up one level. We're still intruding on the GenericDelegator implementation by forcing it to notify us of any changes.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Nicolas Malin (Jira)

[jira] Commented: (OFBIZ-3748) Remove test specific code in the GenericDelegator

In reply to this post by Nicolas Malin (Jira)

[ https://issues.apache.org/jira/browse/OFBIZ-3748?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12863643#action_12863643 ]

Bob Morley commented on OFBIZ-3748:
-----------------------------------

I think separate Factory implementations is appropriate. Ultimately, one would not have the test objects in the production jar at all so there should not be reference to them in the "DelegatorFactoryImpl".

I have read that thread and it does make sense. The trouble is the amount of lengths you had to go through to solve what (at that time and at this current point in time) is still a theoretical requirement. My approach is usually to attempt to create a flexible solution that does its best not to hamstring someone in the future, but does not try to solve problems that currently do not exist. To the best of my knowledge, that probably came out of a lot of years with agile practices. I would have put in your first solution and then if we have a need to execute in parallel solve that problem and the raft of other problems you would encounter. When you go to tackle the problem (test framework execution) you may solve it with something other than parallel execution.

Agree we are intruding on the GenericDelegator implementation; but that is really not my intent. What I would typically do is override just the pieces I wanted to make changes; the trouble is that the implementation of these methods are while not monolithic, they are still rather lengthy. Would rather gut it up a bit more to make it more modular that would allow sub-classes more degrees of freedom. Perhaps it would be better to have the database operations moved into their own methods -- performUpdate, performDelete, performInsert kinda deal and then override in the test delegator and add our test behaviours?

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.