Commons Configuration
  1. Commons Configuration
  2. CONFIGURATION-138

[configuration] HierarchicalConfigurationConverter has problems with attributes

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Nightly Builds
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

      Description

      A problem with correct handling of attributes in
      HierarchicalConfigurationConverter caused the method testAllConfiguration() in
      TestConfigurationFactory to fail if the file "test.xml" was included in the
      configuration definition file.
      The associated patch solves this problem. In testAllConfiguration() also a few
      more asserts have been added to verify that properties have been processed
      correctly.

        Activity

        Hide
        Oliver Heger added a comment -

        Created an attachment (id=12423)
        Patch file for HierarchicalConfigurationConverter and TestConfigurationFactory

        Show
        Oliver Heger added a comment - Created an attachment (id=12423) Patch file for HierarchicalConfigurationConverter and TestConfigurationFactory
        Hide
        Oliver Heger added a comment -

        Created an attachment (id=12424)
        Test configuration definition file with included "test.xml"

        Show
        Oliver Heger added a comment - Created an attachment (id=12424) Test configuration definition file with included "test.xml"
        Hide
        Emmanuel Bourg added a comment -

        Patch applied, thank you ! I'm just a bit concerned about some lines that are
        not covered by the tests though. According to Clover, the condition line 169 is
        never evaluated to false, that means the line 182 is never run and openElements
        doesn't return null. As a result the condition on line 74 is never evaluated to
        true. Could you build a test for this case by chance ?

        Show
        Emmanuel Bourg added a comment - Patch applied, thank you ! I'm just a bit concerned about some lines that are not covered by the tests though. According to Clover, the condition line 169 is never evaluated to false, that means the line 182 is never run and openElements doesn't return null. As a result the condition on line 74 is never evaluated to true. Could you build a test for this case by chance ?
        Hide
        Oliver Heger added a comment -

        Created an attachment (id=12500)
        Additional patch for HierarchicalConfigurationConverter

        Show
        Oliver Heger added a comment - Created an attachment (id=12500) Additional patch for HierarchicalConfigurationConverter
        Hide
        Oliver Heger added a comment -

        I think I found the reason why the lines you pointed out are never executed:
        They are simply unneccessary!

        The original problem was caused by property keys that are a sub string of the
        last key processed by the loop (starting at line 63). For such keys
        openElements() would return null. But actually if such a key appears, it is
        already contained in the keySet and then the continue statement gets executed.
        So openElements() is no more called.

        I attached a patch that removes the affected lines. Clover is a nice tool, I
        didn't see this earlier.

        Oliver

        Show
        Oliver Heger added a comment - I think I found the reason why the lines you pointed out are never executed: They are simply unneccessary! The original problem was caused by property keys that are a sub string of the last key processed by the loop (starting at line 63). For such keys openElements() would return null. But actually if such a key appears, it is already contained in the keySet and then the continue statement gets executed. So openElements() is no more called. I attached a patch that removes the affected lines. Clover is a nice tool, I didn't see this earlier. Oliver
        Hide
        Emmanuel Bourg added a comment -

        Applied, thank you Oliver. Coverage tools are great to track down untested
        features or unnecessary code, I use it all the time

        Show
        Emmanuel Bourg added a comment - Applied, thank you Oliver. Coverage tools are great to track down untested features or unnecessary code, I use it all the time
        Hide
        David Eric Pugh added a comment -

        Have applied the patch, I think the test case makes explicit the behavior expected.

        Show
        David Eric Pugh added a comment - Have applied the patch, I think the test case makes explicit the behavior expected.

          People

          • Assignee:
            Unassigned
            Reporter:
            Oliver Heger
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development