OFBiz
  1. OFBiz
  2. OFBIZ-3670

TestCase base classes providing ability to execute unit tests in Eclipse in "standalone" mode

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: Trunk
    • Fix Version/s: Trunk
    • Component/s: framework
    • Labels:
      None

      Description

      This patch provides the framework that allows junit unit tests to be executed directly in Eclipse (without having to invoke Start directly). It total it contains:

      • BaseTestCase - very top of the TestCase stack containing ofbiz specific assertions
      • StandaloneTestCase - determines if application has executed "Start"; if not it will execute a scaled down "Start unittest" that is used in Eclipse
      • RollbackTestCase - extends standalone and provides a transaction w/ auto-rollback
      • TestRunContainer - provides ability to disable dispatcher attributes via configuration (used for unittest-containers.xml)
      • GeoWorkerTest - unittester that provides "full" coverage of GeoWorker; can be executed from Eclipse or from command-line as it is plugged into standard testdef framework. Creates its own entities with transaction start/rollback so it can be re-executed.

      Would appreciate if this can be a priority so I can provide additional test cases to boost our code coverage that will utilize these features.

        Issue Links

          Activity

          Jacopo Cappellato made changes -
          Status Patch Available [ 10002 ] Closed [ 6 ]
          Resolution Won't Fix [ 2 ]
          Gavin made changes -
          Link This issue is depended upon by OFBIZ-3691 [ OFBIZ-3691 ]
          Gavin made changes -
          Link This issue blocks OFBIZ-3691 [ OFBIZ-3691 ]
          Hide
          Scott Gray added a comment -

          I've tried this already

          I extended the GenericDelegator but it didn't work because the ECAs would get in the way and result in changes being tracked in the wrong order. You are more than welcome to give it a go though, I didn't spend too much time on it and I'm sure there is a solution in there somewhere.

          As to how the existing system works, there isn't actually that much caching of the delegator or retrieving it by name, mostly just in the service dispatcher, job manager and eca handler. Aside from the job manager the others work fine with the current approach because you control their instantiation and can supply the delegator.

          Show
          Scott Gray added a comment - I've tried this already I extended the GenericDelegator but it didn't work because the ECAs would get in the way and result in changes being tracked in the wrong order. You are more than welcome to give it a go though, I didn't spend too much time on it and I'm sure there is a solution in there somewhere. As to how the existing system works, there isn't actually that much caching of the delegator or retrieving it by name, mostly just in the service dispatcher, job manager and eca handler. Aside from the job manager the others work fine with the current approach because you control their instantiation and can supply the delegator.
          Hide
          Adrian Crum added a comment -

          Bob,

          Your ideas kind of tie into the dual build paths mentioned recently. A test-only delegator factory would be specified in a test-only META-INF/services folder.

          Show
          Adrian Crum added a comment - Bob, Your ideas kind of tie into the dual build paths mentioned recently. A test-only delegator factory would be specified in a test-only META-INF/services folder.
          Hide
          Bob Morley added a comment -

          I dug in a little bit further based on your direction and now I see what you are referring to wrt suspended transactions in a new service. Thought of a potential problem with the current approach and would like to run it by you – it appears that in all the code that does a DelegatorFactory.getDelegator will get the original delegator (not the "test" version) and consequently would not rollback. Perhaps I am missing where the test version of the delegator is stuffed into the delegator cache in DelegatorFactory ?

          Reflecting on what the intent seems to be here, I would recommend that we consider a few minor changes.

          1. We consider moving the test specific artifacts from GenericDelegator into either a TestGenericDelegator proxy or sub-class. I am kind of a big proponent of not having test specific code in production code; this proxy/sub-class could live in a org.ofbiz.entity.test package.

          2. We consider a new implementation of DelegatorFactoryImpl which constructs "test" versions of the GenericDelegators. The very nice factory pattern would provide us with the ability to have this new test factory implementation and (hopefully) be able to easily plug it in when we run from the TestRunContainer or from a JUnit test case.

          By doing number 2 what we would ensure is that all delegators created in a container that is for "test" would use test variants, would record their operations, and would "play them back" on rollback. My guess is that we may want to play a rollback on all delegators cached from a test run which would account for any multi-tenancy tests, etc.

          I think we are now on the same page with this work and ensuring that the tests run in a consistent way. If you would like I could take a crack at these proposed changes for your review if you have not had time to start on your updates to my patch. Thoughts?

          Show
          Bob Morley added a comment - I dug in a little bit further based on your direction and now I see what you are referring to wrt suspended transactions in a new service. Thought of a potential problem with the current approach and would like to run it by you – it appears that in all the code that does a DelegatorFactory.getDelegator will get the original delegator (not the "test" version) and consequently would not rollback. Perhaps I am missing where the test version of the delegator is stuffed into the delegator cache in DelegatorFactory ? Reflecting on what the intent seems to be here, I would recommend that we consider a few minor changes. 1. We consider moving the test specific artifacts from GenericDelegator into either a TestGenericDelegator proxy or sub-class. I am kind of a big proponent of not having test specific code in production code; this proxy/sub-class could live in a org.ofbiz.entity.test package. 2. We consider a new implementation of DelegatorFactoryImpl which constructs "test" versions of the GenericDelegators. The very nice factory pattern would provide us with the ability to have this new test factory implementation and (hopefully) be able to easily plug it in when we run from the TestRunContainer or from a JUnit test case. By doing number 2 what we would ensure is that all delegators created in a container that is for "test" would use test variants, would record their operations, and would "play them back" on rollback. My guess is that we may want to play a rollback on all delegators cached from a test run which would account for any multi-tenancy tests, etc. I think we are now on the same page with this work and ensuring that the tests run in a consistent way. If you would like I could take a crack at these proposed changes for your review if you have not had time to start on your updates to my patch. Thoughts?
          Hide
          Scott Gray added a comment -

          For #1, transaction rollbacks only work for simple use cases. It is not uncommon for OFBiz services to run in their own transaction which would suspend your transaction and then resume it once it is done, this is the reason for the system we have in place today. I don't want solutions that only work some of the time when we have one already that works (almost) all of the time.

          I'm going to make some changes to your patch that incorporates what you are trying to do into the existing test tools, I'll post it here when I'm done and we can discuss it further.

          Show
          Scott Gray added a comment - For #1, transaction rollbacks only work for simple use cases. It is not uncommon for OFBiz services to run in their own transaction which would suspend your transaction and then resume it once it is done, this is the reason for the system we have in place today. I don't want solutions that only work some of the time when we have one already that works (almost) all of the time. I'm going to make some changes to your patch that incorporates what you are trying to do into the existing test tools, I'll post it here when I'm done and we can discuss it further.
          Scott Gray made changes -
          Assignee Scott Gray [ lektran ]
          Hide
          Bob Morley added a comment -

          Hey Scott,

          1) This base test case uses the standard transaction rollback system.

          2) The individual writing the unit test can decide what they should extend – I am not suggesting that all unit tests should extend the RollbackTestCase; they may extend the StandaloneTestCase (which does not have the rollback nature). There were tests in my travels which were explicitly calling a "rollback" at the end of their test which this pattern would avoid. I would also argue that it is bad practice for having dependency in your test classes in the first place.

          3) This is not providing a different approach at all; it merely provides extra functionality into the exist test framework. The tests remain in the testdef and are executed through standard means, you simply have the ability to execute the test immediately from Eclipse and get all of the tooling associated with that.

          There is a method of executing the TestRunContainer to pass in arguments on which test to run. It is cumbersome to use in the IDE; but if people choose to execute their tests in that way then there is nothing about this patch that prevents that. In fact, I write the unit test and execute it through eclipse (individual methods, the whole class) get the tooling in Eclipse (or any other IDE) and then I add it to the testdef and execute the entire suite of tests form the command-line before creating any patch.

          Show
          Bob Morley added a comment - Hey Scott, 1) This base test case uses the standard transaction rollback system. 2) The individual writing the unit test can decide what they should extend – I am not suggesting that all unit tests should extend the RollbackTestCase; they may extend the StandaloneTestCase (which does not have the rollback nature). There were tests in my travels which were explicitly calling a "rollback" at the end of their test which this pattern would avoid. I would also argue that it is bad practice for having dependency in your test classes in the first place. 3) This is not providing a different approach at all; it merely provides extra functionality into the exist test framework. The tests remain in the testdef and are executed through standard means, you simply have the ability to execute the test immediately from Eclipse and get all of the tooling associated with that. There is a method of executing the TestRunContainer to pass in arguments on which test to run. It is cumbersome to use in the IDE; but if people choose to execute their tests in that way then there is nothing about this patch that prevents that. In fact, I write the unit test and execute it through eclipse (individual methods, the whole class) get the tooling in Eclipse (or any other IDE) and then I add it to the testdef and execute the entire suite of tests form the command-line before creating any patch.
          Hide
          Scott Gray added a comment -

          Hi Bob,

          I don't really like this approach for a couple of reasons:
          1. We already have a rollback system in place
          2. OFBiz allows unit tests to depend on each other within a suite which your approach won't allow because of each unit test doing a rollback
          3. If this is committed then we'll end up with two different approaches to testing and it will be more complicated for people to write new tests

          Why not just improve the TestRunContainer to allow it to start OFBiz and pass in arguments of what tests to run?

          Show
          Scott Gray added a comment - Hi Bob, I don't really like this approach for a couple of reasons: 1. We already have a rollback system in place 2. OFBiz allows unit tests to depend on each other within a suite which your approach won't allow because of each unit test doing a rollback 3. If this is committed then we'll end up with two different approaches to testing and it will be more complicated for people to write new tests Why not just improve the TestRunContainer to allow it to start OFBiz and pass in arguments of what tests to run?
          Scott Gray made changes -
          Link This issue blocks OFBIZ-3691 [ OFBIZ-3691 ]
          Bob Morley made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Bob Morley made changes -
          Field Original Value New Value
          Attachment OFBIZ-3670_StandaloneTestEnhancement.patch [ 12441379 ]
          Bob Morley created issue -

            People

            • Assignee:
              Scott Gray
              Reporter:
              Bob Morley
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development