Camel
  1. Camel
  2. CAMEL-4538

Make CamelTestSupport return Registry instead of JndiRegistry in createRegistry method

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.8.0
    • Fix Version/s: 3.0.0
    • Component/s: tests
    • Labels:
      None
    • Patch Info:
      Patch Available
    • Estimated Complexity:
      Moderate

      Description

      Usually when we write camel unit tests we want to use SimpleRegistry, not JndiRegistry

      1. register_last.diff
        185 kB
        Bilgin Ibryam
      2. CAMEL-4538-v2
        3 kB
        Maciej Prochniak
      3. CAMEL-4538
        1 kB
        Maciej Prochniak

        Activity

        Hide
        Claus Ibsen added a comment -

        There is also a TestSupport in junit4 package, as well in camel-testng. So we need to change in all places.
        Do you mind creating a new patch with these changes?

        Show
        Claus Ibsen added a comment - There is also a TestSupport in junit4 package, as well in camel-testng. So we need to change in all places. Do you mind creating a new patch with these changes?
        Hide
        Maciej Prochniak added a comment -

        Sure, I'll do it, thanks for pointing this

        Show
        Maciej Prochniak added a comment - Sure, I'll do it, thanks for pointing this
        Hide
        Maciej Prochniak added a comment -

        here it is

        Show
        Maciej Prochniak added a comment - here it is
        Hide
        Claus Ibsen added a comment -

        Thanks for the new patch. Unfortunately there is about 20 Camel components and a few examples etc. that need to be changed as well to compile.
        Basically they need that type cast. If you don't mind creating a new patch with those code compile fixes, then that would be great.

        Basically you can compile the entire source code as documented here
        http://camel.apache.org/building.html

        For example the quick build. I am not sure if it even compiles the unit tests, so I normally run this command

        mvn clean install -Dtest=false
        

        I guess old habits die hard.

        Show
        Claus Ibsen added a comment - Thanks for the new patch. Unfortunately there is about 20 Camel components and a few examples etc. that need to be changed as well to compile. Basically they need that type cast. If you don't mind creating a new patch with those code compile fixes, then that would be great. Basically you can compile the entire source code as documented here http://camel.apache.org/building.html For example the quick build. I am not sure if it even compiles the unit tests, so I normally run this command mvn clean install -Dtest= false I guess old habits die hard.
        Hide
        Claus Ibsen added a comment -

        To use SimpleRegistry, you override createCamelContext instead and create a context with the simple registry.

        Show
        Claus Ibsen added a comment - To use SimpleRegistry, you override createCamelContext instead and create a context with the simple registry.
        Hide
        Willem Jiang added a comment -

        Maybe we can change the interface in Camel 3.x.

        Show
        Willem Jiang added a comment - Maybe we can change the interface in Camel 3.x.
        Hide
        Bilgin Ibryam added a comment -

        I think I got it wrong because if we replace JndiRegistry with Registry in createRegistry method, then we have to cast it back to JndiRegistry in all the test, because Registry doesn't have a bind/put method and it is used in all the tests.

        May be replacing JndiRegistry with SimpleRegistry is the intend of this issue?

        Show
        Bilgin Ibryam added a comment - I think I got it wrong because if we replace JndiRegistry with Registry in createRegistry method, then we have to cast it back to JndiRegistry in all the test, because Registry doesn't have a bind/put method and it is used in all the tests. May be replacing JndiRegistry with SimpleRegistry is the intend of this issue?
        Hide
        Claus Ibsen added a comment -

        Ideally we should use SimpleRegistry by default in the camel-test kit.
        However due JndiRegistry was the 1st then it was used as default.

        What people want in their unit tests is to easy add a entry in the registry.
        And yes I think we should use SimpleRegistry by default, so that means maybe X unit tests needs to be changed.

        Show
        Claus Ibsen added a comment - Ideally we should use SimpleRegistry by default in the camel-test kit. However due JndiRegistry was the 1st then it was used as default. What people want in their unit tests is to easy add a entry in the registry. And yes I think we should use SimpleRegistry by default, so that means maybe X unit tests needs to be changed.
        Hide
        Bilgin Ibryam added a comment -

        Currently JndiRegistry uses JndiContext and class loading from jndi.properties ... any idea how to change/fix these tests?

        Show
        Bilgin Ibryam added a comment - Currently JndiRegistry uses JndiContext and class loading from jndi.properties ... any idea how to change/fix these tests?
        Hide
        Christian Müller added a comment -

        +1 I would like to see this change.

        Show
        Christian Müller added a comment - +1 I would like to see this change.
        Hide
        Bilgin Ibryam added a comment -

        In order to continue with this task I need some answers and direction.

        1. In CAMEL-4952 - it mentions adding beans to the registry, does it mean extending the Registry interface with methods for binding objects. If the answer is yes, I wonder why do we need createJndiContext method, it would be simpler to bind and lookup directly from the Registry.

        2. My initial idea was to fix all the tests by:
        a) replacing all data binding to JndiRegistry with data binding to JndiContext
        b) replace JndiRegistry with Registry/SimpleRegistry

        but even with these changes there still would be JndiContext and creating a SimpleRegistry with JndiContext is ugly.
        I'm out of ideas for now

        Show
        Bilgin Ibryam added a comment - In order to continue with this task I need some answers and direction. 1. In CAMEL-4952 - it mentions adding beans to the registry, does it mean extending the Registry interface with methods for binding objects. If the answer is yes, I wonder why do we need createJndiContext method, it would be simpler to bind and lookup directly from the Registry. 2. My initial idea was to fix all the tests by: a) replacing all data binding to JndiRegistry with data binding to JndiContext b) replace JndiRegistry with Registry/SimpleRegistry but even with these changes there still would be JndiContext and creating a SimpleRegistry with JndiContext is ugly. I'm out of ideas for now
        Hide
        Bilgin Ibryam added a comment -

        1. Introduced SimpleContext as data holder for SimpleRegitry()

        2. Updated the tests to use createSimpleContext() instead of createRegistry() for binding data to the registry.

        3. Refactored createRegistry() to return Registry instead of JndiRegistry

        With these changes tests by default will use SimpleRegistry, but still will be able to JndiRegistrty when needed, by overriding the createRegistry() and use createJndiContext() method.

        WDYT?

        Show
        Bilgin Ibryam added a comment - 1. Introduced SimpleContext as data holder for SimpleRegitry() 2. Updated the tests to use createSimpleContext() instead of createRegistry() for binding data to the registry. 3. Refactored createRegistry() to return Registry instead of JndiRegistry With these changes tests by default will use SimpleRegistry, but still will be able to JndiRegistrty when needed, by overriding the createRegistry() and use createJndiContext() method. WDYT?
        Hide
        Bilgin Ibryam added a comment -

        Anyone with comments? The patch changes a lot of tests, I guess it will be out of date soon, unless someone has a look.

        Show
        Bilgin Ibryam added a comment - Anyone with comments? The patch changes a lot of tests, I guess it will be out of date soon, unless someone has a look.
        Hide
        Hadrian Zbarcea added a comment -

        @Bilgin, I just took a look and I have a few questions. Why was the SimpleContext introduced? The SimpleRegistry would do equally well. Plus adding the SimpleContext in the o.a.camel package is probably not the best idea. Also the SimpleRegistry is not really a holder for the SimpleContext, but a copy, right?

        What about having createRegistry() return a Registry (and maybe add a couple of methods to it for adding/removing entries?

        Show
        Hadrian Zbarcea added a comment - @Bilgin, I just took a look and I have a few questions. Why was the SimpleContext introduced? The SimpleRegistry would do equally well. Plus adding the SimpleContext in the o.a.camel package is probably not the best idea. Also the SimpleRegistry is not really a holder for the SimpleContext, but a copy, right? What about having createRegistry() return a Registry (and maybe add a couple of methods to it for adding/removing entries?
        Hide
        Claus Ibsen added a comment -

        Agree with Hadrian, this ticket should not introduce new APIs in camel-core. Its a pure change in camel-test.

        And yes createRegistry should just return a Registry interface. Then people can override the createRegistry method and return their impl of choice. In fact we should consider using SimpleRegistry as the default now instead of JndiRegistry.

        And we could have some helper methods on CamelTestSupport to add a bean to the registry. Then the impl can check if the registry is a SimpleRegistry, JndiRegistry and cast to the type, and use the put/bind methods. And for other registry types such as the Spring AppCtx, then it can throw an exception saying this is not supported.

        Show
        Claus Ibsen added a comment - Agree with Hadrian, this ticket should not introduce new APIs in camel-core. Its a pure change in camel-test. And yes createRegistry should just return a Registry interface. Then people can override the createRegistry method and return their impl of choice. In fact we should consider using SimpleRegistry as the default now instead of JndiRegistry. And we could have some helper methods on CamelTestSupport to add a bean to the registry. Then the impl can check if the registry is a SimpleRegistry, JndiRegistry and cast to the type, and use the put/bind methods. And for other registry types such as the Spring AppCtx, then it can throw an exception saying this is not supported.
        Hide
        Bilgin Ibryam added a comment -

        Thanks for your feedback, this is what I needed. I've updated the patch according to the comments. I will provide my comments later, uploading a patch for now.

        Show
        Bilgin Ibryam added a comment - Thanks for your feedback, this is what I needed. I've updated the patch according to the comments. I will provide my comments later, uploading a patch for now.
        Hide
        Bilgin Ibryam added a comment -

        @Hadrian I introduced SimpleContext because noticed that each registry has a context:
        JndiRegistry - JndiContext, ApplicationContextRegistry - ApplicationContext, OsgiServiceRegistry - BundleContext…
        All that made me think that each registry is a kind of read only access interface (only lookup methods, without any bind methods) to the specific context.
        Adding some add/remove methods to the Registry would simplify this task, but I think that is a major change for the Registry interface. And yes, the location for SimpleContext was wrong…

        @Claus I didn't want to do any API changes in Registry that's why introduced SimpleContext, which is also API change by introducing a new class

        Anyway, all this is past, because after your comments I updated the patch, and there are no api changes any more.

        The patch does the following:

        1. createRegistry() return a Registry, with SimpleRegistry implementation by default.
        2. Tests override this method to add beans to SimpleRegistry. That is possible because SimpleRegistry is also a Map.

        Show
        Bilgin Ibryam added a comment - @Hadrian I introduced SimpleContext because noticed that each registry has a context: JndiRegistry - JndiContext, ApplicationContextRegistry - ApplicationContext, OsgiServiceRegistry - BundleContext… All that made me think that each registry is a kind of read only access interface (only lookup methods, without any bind methods) to the specific context. Adding some add/remove methods to the Registry would simplify this task, but I think that is a major change for the Registry interface. And yes, the location for SimpleContext was wrong… @Claus I didn't want to do any API changes in Registry that's why introduced SimpleContext, which is also API change by introducing a new class Anyway, all this is past, because after your comments I updated the patch, and there are no api changes any more. The patch does the following: 1. createRegistry() return a Registry, with SimpleRegistry implementation by default. 2. Tests override this method to add beans to SimpleRegistry. That is possible because SimpleRegistry is also a Map.
        Hide
        Bilgin Ibryam added a comment -

        Can someone have a quick look at the updated patch?

        Show
        Bilgin Ibryam added a comment - Can someone have a quick look at the updated patch?
        Hide
        Claus Ibsen added a comment -

        There is also a camel-testng that may need changes?

        If there is no API changes in camel-core, then the patch seems okay.

        Show
        Claus Ibsen added a comment - There is also a camel-testng that may need changes? If there is no API changes in camel-core, then the patch seems okay.
        Hide
        Claus Ibsen added a comment -

        Bilgin, you will be able to apply this patch yourself soon

        Show
        Claus Ibsen added a comment - Bilgin, you will be able to apply this patch yourself soon
        Hide
        Claus Ibsen added a comment -

        Lets not break any API in the 2.x

        Show
        Claus Ibsen added a comment - Lets not break any API in the 2.x

          People

          • Assignee:
            Bilgin Ibryam
            Reporter:
            Maciej Prochniak
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development