Details

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

      Description

      Back up all existing implementations with unit tests:

      • Argument
      • Assertions
      • BeanUtils
      • DefaultBeanAccessor
      • DefaultClassAccessor
      • MethodRegistry
      • TypeUtils
      1. SANDBOX-362-TypeUtilsTest.txt
        70 kB
        Benedikt Ritter
      2. SANDBOX-362-RefactoringOfAssertionsTest.txt
        9 kB
        Benedikt Ritter
      3. SANDBOX-362-RefactoringOfArgumentTest.txt
        25 kB
        Benedikt Ritter
      4. SANDBOX-362-ConstructorTestCase.txt
        8 kB
        Benedikt Ritter
      5. SANDBOX-362-BeanUtilsTest.txt
        6 kB
        Benedikt Ritter
      6. ArgumentTest.zip
        2 kB
        Benedikt Ritter

        Activity

        Hide
        Benedikt Ritter added a comment -

        I've attached a jUnit Test for Argument. Please let me know, what you think about it. I've not written test methods for getType() and getValue(), since those are backed up by the other methods.

        Show
        Benedikt Ritter added a comment - I've attached a jUnit Test for Argument. Please let me know, what you think about it. I've not written test methods for getType() and getValue(), since those are backed up by the other methods.
        Hide
        Simone Tripodi added a comment -

        Your contribution has been applied Benedikt, thanks a lot! see r1234115

        I have to ask you anyway the favor of, for future contributions, read and follow the On Contributing Patches guide, widely adopted in commons.

        For that case I applied the contribution since it is an addition class, but when modifying the original source code, the patch fits better because shows what has been modified.

        Dankeshön!!!

        Show
        Simone Tripodi added a comment - Your contribution has been applied Benedikt, thanks a lot! see r1234115 I have to ask you anyway the favor of, for future contributions, read and follow the On Contributing Patches guide, widely adopted in commons. For that case I applied the contribution since it is an addition class, but when modifying the original source code, the patch fits better because shows what has been modified. Dankeshön!!!
        Hide
        Benedikt Ritter added a comment - - edited

        Based on a discussion we had on the mailing list about best practices for unit tests, I refactored ArgumentTest and added test methods for getParamaters(Argument<?>... args).

        Here is a summary of what I changed:

        • separated long methods into very small methods: each primitive type has it's own test methods
        • defined fields for every primitive type, to make sure that assertions do not accedential fail because wrong values get compared
        • created separate test methods instead of wrapping failure cases inside try/catch-blocks
        • Added tests methods that test against TestBean.class

        Please let me know, what you think about the changes!

        Show
        Benedikt Ritter added a comment - - edited Based on a discussion we had on the mailing list about best practices for unit tests, I refactored ArgumentTest and added test methods for getParamaters(Argument<?>... args). Here is a summary of what I changed: separated long methods into very small methods: each primitive type has it's own test methods defined fields for every primitive type, to make sure that assertions do not accedential fail because wrong values get compared created separate test methods instead of wrapping failure cases inside try/catch-blocks Added tests methods that test against TestBean.class Please let me know, what you think about the changes!
        Hide
        Simone Tripodi added a comment -

        Terrific I have already applied your patch, see r1235012, thanks for contributing!!!

        Show
        Simone Tripodi added a comment - Terrific I have already applied your patch, see r1235012 , thanks for contributing!!!
        Hide
        Benedikt Ritter added a comment -

        Hi,

        I have created a patch that adds a unit test for type utils. The test consists of 244 test methods. To be honest, it has been a lot of copy and paste work. But I think that TypeUtils is one of the central classes, so one test more or less doesn't hurt (or does it?). I wanted to test at least every possible combination of primtives and wrapper types. If you think, that the test is to huge, just let me know!

        @Simo: if you apply the patch, think very carefully if you want to change TypeUtils afterwards. It will beak a bazillion tests

        Show
        Benedikt Ritter added a comment - Hi, I have created a patch that adds a unit test for type utils. The test consists of 244 test methods. To be honest, it has been a lot of copy and paste work. But I think that TypeUtils is one of the central classes, so one test more or less doesn't hurt (or does it?). I wanted to test at least every possible combination of primtives and wrapper types. If you think, that the test is to huge, just let me know! @Simo: if you apply the patch, think very carefully if you want to change TypeUtils afterwards. It will beak a bazillion tests
        Hide
        Benedikt Ritter added a comment -

        Forgot to mention: I also added some not null checks to TypeUtil.

        Show
        Benedikt Ritter added a comment - Forgot to mention: I also added some not null checks to TypeUtil.
        Hide
        Simone Tripodi added a comment -

        hey, I usually run tests before committing!

        Show
        Simone Tripodi added a comment - hey, I usually run tests before committing!
        Hide
        Simone Tripodi added a comment -

        thanks again for your contribution!!! patch has been successfully applied, see r1235494

        looking forward to the next!

        Show
        Simone Tripodi added a comment - thanks again for your contribution!!! patch has been successfully applied, see r1235494 looking forward to the next!
        Hide
        Benedikt Ritter added a comment -

        As announced on the mailing list, I refactored AssertionsTest Summary:

        • created smaller methods for different failure cases
        • added an ExpectedException test rule to validate correct errorTemplate and errorArgs processing

        moving on to the next class

        Show
        Benedikt Ritter added a comment - As announced on the mailing list, I refactored AssertionsTest Summary: created smaller methods for different failure cases added an ExpectedException test rule to validate correct errorTemplate and errorArgs processing moving on to the next class
        Hide
        Simone Tripodi added a comment -

        very good patch Benedict, it has been successfully applied, see r1236622

        thanks!

        Show
        Simone Tripodi added a comment - very good patch Benedict, it has been successfully applied, see r1236622 thanks!
        Hide
        Benedikt Ritter added a comment -

        I have created a very basic Test for BeanUtils. All it does is testing if the on-methods return a result != null for every primitive and every wrapper type. Any ideas what else we could test regarding BeanUtils class?

        The test is a bit redundant, because the on-methods get tested in ConstructorTestCase and StaticMethodTestCase as well. But since BeanUtils serves as the entry point for the hole library, I thought we should be absolutely sure that everything is working correct there.

        Show
        Benedikt Ritter added a comment - I have created a very basic Test for BeanUtils. All it does is testing if the on -methods return a result != null for every primitive and every wrapper type. Any ideas what else we could test regarding BeanUtils class? The test is a bit redundant, because the on -methods get tested in ConstructorTestCase and StaticMethodTestCase as well. But since BeanUtils serves as the entry point for the hole library, I thought we should be absolutely sure that everything is working correct there.
        Hide
        Simone Tripodi added a comment -

        Hi Benedikt,

        I think that redundancies are not evil but in that case they are not very helpful because they don't change the test coverage result.

        BeanUtils APIs tests purpose should be:

        • make sure null Class is not accepted
        • make sure null Object is not accepted
        • accepts a Class
        • accepts a Bean

        nothing more, unless we decide to add more public methods.

        Show
        Simone Tripodi added a comment - Hi Benedikt, I think that redundancies are not evil but in that case they are not very helpful because they don't change the test coverage result. BeanUtils APIs tests purpose should be: make sure null Class is not accepted make sure null Object is not accepted accepts a Class accepts a Bean nothing more, unless we decide to add more public methods.
        Hide
        Benedikt Ritter added a comment -

        Hi again,

        I have attached a patch for ConstructorTestCase. Summary:

        • moved expected failure cases to own test methods
        • added test methods for null arguments
        • added ExceptedException test rule to verify that correct exceptions are thrown

        I still don't like, that some methods test more than one constructor. Maybe we should create separate methods for those.

        I also changed an exception message in DefaultClassAccessor from
        "No such accessible constructor on object:"
        to
        "No such accessible constructor on class:"
        because I think that this is more convenient.

        Regards
        Benedikt

        Show
        Benedikt Ritter added a comment - Hi again, I have attached a patch for ConstructorTestCase. Summary: moved expected failure cases to own test methods added test methods for null arguments added ExceptedException test rule to verify that correct exceptions are thrown I still don't like, that some methods test more than one constructor. Maybe we should create separate methods for those. I also changed an exception message in DefaultClassAccessor from "No such accessible constructor on object:" to "No such accessible constructor on class:" because I think that this is more convenient. Regards Benedikt
        Hide
        Simone Tripodi added a comment -

        Hi Benedikt,

        great. Since classes will increase in therms of number, it would be better for, future contributions , to open subtasks and attach there patches, otherwise it is confusing to keept track which patches have been applied and which not.

        Thanks for your contributions!

        Show
        Simone Tripodi added a comment - Hi Benedikt, great. Since classes will increase in therms of number, it would be better for, future contributions , to open subtasks and attach there patches, otherwise it is confusing to keept track which patches have been applied and which not. Thanks for your contributions!
        Hide
        Benedikt Ritter added a comment -

        All subtasks have been resolved.

        Show
        Benedikt Ritter added a comment - All subtasks have been resolved.

          People

          • Assignee:
            Simone Tripodi
            Reporter:
            Benedikt Ritter
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development