You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Benedikt Ritter (Created) (JIRA)" <ji...@apache.org> on 2012/02/01 22:20:58 UTC
[jira] [Created] (SANDBOX-379) [BeanUtils2] Implement describe() on
DefaultBeanAccessor
[BeanUtils2] Implement describe() on DefaultBeanAccessor
---------------------------------------------------------
Key: SANDBOX-379
URL: https://issues.apache.org/jira/browse/SANDBOX-379
Project: Commons Sandbox
Issue Type: Improvement
Components: BeanUtils2
Affects Versions: Nightly Builds
Reporter: Benedikt Ritter
Implement the above mentioned method an corresponding unit tests
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (SANDBOX-379) [BeanUtils2] Implement describe()
on DefaultBeanAccessor
Posted by "Benedikt Ritter (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SANDBOX-379?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13199622#comment-13199622 ]
Benedikt Ritter commented on SANDBOX-379:
-----------------------------------------
As I said above, {{describe()}}, {{populate()}}, {{clone()}}, etc are the endpoints of a fluent call. So I thought putting them together in one test is a good idea.
{quote}
It would be more obvious creating a serie of DescribeTestCase
{quote}
Okay, so do we create a test case for every method on {{BeanAccessor}}? I think that would be a good idea. I don't like huge test classes. And a test class for all methods on {{BeanAccessor}} would be gigantic (as you mentioned).
> [BeanUtils2] Implement describe() on DefaultBeanAccessor
> ---------------------------------------------------------
>
> Key: SANDBOX-379
> URL: https://issues.apache.org/jira/browse/SANDBOX-379
> Project: Commons Sandbox
> Issue Type: Improvement
> Components: BeanUtils2
> Affects Versions: Nightly Builds
> Reporter: Benedikt Ritter
> Attachments: SANDBOX-379.txt
>
>
> Implement the above mentioned method an corresponding unit tests
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (SANDBOX-379) [BeanUtils2] Implement describe()
on DefaultBeanAccessor
Posted by "Benedikt Ritter (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SANDBOX-379?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13198694#comment-13198694 ]
Benedikt Ritter commented on SANDBOX-379:
-----------------------------------------
Hi again,
{quote}so split per-functionalities, {{DefaultBeanAccessor}} would contain the biggest amount of tests{quote}
Not necessarily. For example we created test cases for {{getProperty()}} and {{setProperty()}} and for {{invoke(Exact)Method}}, even though they are part of {{BeanAccessor}} (we could have added that to BeanUtilsTest as well). I think it would be a good idea to test all methods of a class that end the fluent API call in one test class and just make sure that fluent API methods never return null and throw appropriate exceptions.
So, what am I talking about? For exmaple {{setProperty()}} returns a {{BeanPropertySetter}} on which we can continue to fluently call methods. The same applies for {{invoke(Exact)Method()}}. In {{BeanAccessorTest}} I would just make sure, that BeanAccessor.setProperty() never returns null and throws appropriate Exceptions for invalid arguments. That is the reason, why I added BeanUtilsTest in the first place. It is supposed to make sure, that changes to BeanUtils will never break the chain of fluent calls (although you were right, when you said, that it does not add much regarding the total test coverage).
Beside that I would make sure that {{describe()}}, {{cloneBean}}, {{populate()}} etc. work correct, since they end the chain of fluent calls by returning POJOs.
Different scenarios for {{setProperty().withValue()}} are a different concern, so we have a separated test for that. The only think that does not fit in that schema is {{getProperty()}}. But since it is a very important functionality, I think a separate test makes sense.
So, that's my big picture for the test environment... If you like that, I would just move the contents of {{Jira157Test}} to {{BeanAccessorTest}} and leave the rest unchanged.
{quote}
it's not a matter of taste, sometimes (depending on the bug) I do the same in projects that have already been released, not in an experiment. The bug is known and fixed in a previous version of the component, references in comments are fine but don't see the advantage of dedicated test in a new version.
{quote}
Okay, I'll move it :)
> [BeanUtils2] Implement describe() on DefaultBeanAccessor
> ---------------------------------------------------------
>
> Key: SANDBOX-379
> URL: https://issues.apache.org/jira/browse/SANDBOX-379
> Project: Commons Sandbox
> Issue Type: Improvement
> Components: BeanUtils2
> Affects Versions: Nightly Builds
> Reporter: Benedikt Ritter
> Attachments: SANDBOX-379.txt
>
>
> Implement the above mentioned method an corresponding unit tests
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (SANDBOX-379) [BeanUtils2] Implement describe()
on DefaultBeanAccessor
Posted by "Benedikt Ritter (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SANDBOX-379?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13198674#comment-13198674 ]
Benedikt Ritter commented on SANDBOX-379:
-----------------------------------------
Hey, thanks for the feedback. Let me comment on that ;)
{quote}
{{BeanAccessorTestCase}} has only 1 test method that can be included in BeanUtilsTest; the describes is protected (and there's no reason why), it can be {{private final}}.
{quote}
I created BeanAccessorTestCase as a Unit Test for DefaultBeanAccessor (wow, supprise ;) ), having in mind, that there are some more methods to be tested like clone(), populate() and copyProperties(). BeanUtilsTest is a unit test for testing the factory methods in BeanUtils. Adding tests for all methods in BeanAccessor to BeanUtilsTest would lead to a huge test class, that tests more than one class at once. So I thought creating a new test for DefaultBeanAccessor would be a good idea, regarding separation of concerns. What do you think?
The protected String array was copied from BeanUtils1, but you're right, there is no reason to have it protected. I'll change it.
{quote}
Same thing for {{Jira157TestCase}}, test methods can be moved in the {{BeanUtilsTest}} class; methods name have to be renamed stripping the {{testIssue_BEANUTILS_157_BeanUtils}} prefix;
{quote}
I really liked the idea to have a dedicated test for a bug. To be honest, I was thinking about proposing that in my company :) But if you don't like it, I'll move it depending on your comment to the above mentioned topic.
{quote}
{{AccessibleObjectsRegistry#getMethodsRegistry()}} can be referenced statically in {{PropertyDescriptorsRegistry;}}
{quote}
I'll change it.
{quote}
{{PropertyDescriptorsRegistry#makeMethodsAccessible}} returns the same {{PropertyDescriptor}} input instance, there's no reason that method has a return statement, signature can be changed to void.
{quote}
ok :)
Have a nice day!
Benedikt
> [BeanUtils2] Implement describe() on DefaultBeanAccessor
> ---------------------------------------------------------
>
> Key: SANDBOX-379
> URL: https://issues.apache.org/jira/browse/SANDBOX-379
> Project: Commons Sandbox
> Issue Type: Improvement
> Components: BeanUtils2
> Affects Versions: Nightly Builds
> Reporter: Benedikt Ritter
> Attachments: SANDBOX-379.txt
>
>
> Implement the above mentioned method an corresponding unit tests
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (SANDBOX-379) [BeanUtils2] Implement describe() on
DefaultBeanAccessor
Posted by "Benedikt Ritter (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SANDBOX-379?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Benedikt Ritter updated SANDBOX-379:
------------------------------------
Attachment: SANDBOX-379.txt
I have created a patch for this issue. Summary:
* Added implementation to DefaultBeanAccessor
* Modified PropertyDescriptorRegistry to fit the needs of describe()
* Added JavaDoc from BeanUtils1 to BeanAccessor Interface
* Copied and adjusted Jira157TestCase
* Added logic to PropertyDescriptorRegistry to make sure getter and setter are accessbile (workaround for BEANUTILS-157 extracted from BeanUtils1)
I hope this time everything is fine with formating etc ;)
Regards
Benedikt
> [BeanUtils2] Implement describe() on DefaultBeanAccessor
> ---------------------------------------------------------
>
> Key: SANDBOX-379
> URL: https://issues.apache.org/jira/browse/SANDBOX-379
> Project: Commons Sandbox
> Issue Type: Improvement
> Components: BeanUtils2
> Affects Versions: Nightly Builds
> Reporter: Benedikt Ritter
> Attachments: SANDBOX-379.txt
>
>
> Implement the above mentioned method an corresponding unit tests
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Resolved] (SANDBOX-379) [BeanUtils2] Implement describe()
on DefaultBeanAccessor
Posted by "Simone Tripodi (Resolved) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SANDBOX-379?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Simone Tripodi resolved SANDBOX-379.
------------------------------------
Resolution: Fixed
Assignee: Simone Tripodi
Patch (finally) applied, see [r1240336|http://svn.apache.org/viewvc?rev=1240336&view=rev], thanks for the contribution.
I applied 2 minor modifications:
* misplaced {{DescribeTestCase}} license header;
* {{PropertyDescriptorsRegistry#makeMethodsAccessible}} can be static
> [BeanUtils2] Implement describe() on DefaultBeanAccessor
> ---------------------------------------------------------
>
> Key: SANDBOX-379
> URL: https://issues.apache.org/jira/browse/SANDBOX-379
> Project: Commons Sandbox
> Issue Type: Improvement
> Components: BeanUtils2
> Affects Versions: Nightly Builds
> Reporter: Benedikt Ritter
> Assignee: Simone Tripodi
> Attachments: SANDBOX-379.txt, SANDBOX-379v2.txt
>
>
> Implement the above mentioned method an corresponding unit tests
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (SANDBOX-379) [BeanUtils2] Implement describe()
on DefaultBeanAccessor
Posted by "Simone Tripodi (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SANDBOX-379?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13199620#comment-13199620 ]
Simone Tripodi commented on SANDBOX-379:
----------------------------------------
We do have {{GetPropertyTestCase}}, {{MethodsTestCase}}, {{SetPropertyTestCase}}, can you explain what's the reason why the {{describe}} tests have to be in {{BeanAccessorTestCase}}?
It would be more obvious creating a serie of {{DescribeTestCase}}
> [BeanUtils2] Implement describe() on DefaultBeanAccessor
> ---------------------------------------------------------
>
> Key: SANDBOX-379
> URL: https://issues.apache.org/jira/browse/SANDBOX-379
> Project: Commons Sandbox
> Issue Type: Improvement
> Components: BeanUtils2
> Affects Versions: Nightly Builds
> Reporter: Benedikt Ritter
> Attachments: SANDBOX-379.txt
>
>
> Implement the above mentioned method an corresponding unit tests
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (SANDBOX-379) [BeanUtils2] Implement describe() on
DefaultBeanAccessor
Posted by "Benedikt Ritter (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SANDBOX-379?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Benedikt Ritter updated SANDBOX-379:
------------------------------------
Attachment: SANDBOX-379v2.txt
Patch modified. Summary:
* renamed {{BeanAccessorTestCase}} to {{DescribeTestCase}} and moved test methods from {{Jira157TestCase}} there
* Added static reference to {{MethodsRegistry}} in {{PropertyDescriptorRegistry}}
* Changed signature of {{PropertyDescrioptorsRegistry.makeMethodsAccessible()}} to {{void}}
Please have a close look at line 144 in {{PropertyDescriprotrsRegistry}}:
{code:java}
readMethod = METHODS_REGISTRY.get( true, beanType, readMethod.getName() );
{code}
I'm afraid that this will get us into trouble when we implement the methods for indexed and mapped properties (because I'm not passing any arguments).
Thank you for the inspiring discussion on this issue!
> [BeanUtils2] Implement describe() on DefaultBeanAccessor
> ---------------------------------------------------------
>
> Key: SANDBOX-379
> URL: https://issues.apache.org/jira/browse/SANDBOX-379
> Project: Commons Sandbox
> Issue Type: Improvement
> Components: BeanUtils2
> Affects Versions: Nightly Builds
> Reporter: Benedikt Ritter
> Attachments: SANDBOX-379.txt, SANDBOX-379v2.txt
>
>
> Implement the above mentioned method an corresponding unit tests
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (SANDBOX-379) [BeanUtils2] Implement describe()
on DefaultBeanAccessor
Posted by "Simone Tripodi (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SANDBOX-379?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13198681#comment-13198681 ]
Simone Tripodi commented on SANDBOX-379:
----------------------------------------
hola,
{quote}
So I thought creating a new test for DefaultBeanAccessor would be a good idea, regarding separation of concerns. What do you think?
{quote}
so split per-functionalities, {{DefaultBeanAccessor}} would contain the biggest amount of tests
{quote}
I really liked the idea to have a dedicated test for a bug. To be honest, I was thinking about proposing that in my company But if you don't like it, I'll move it depending on your comment to the above mentioned topic.
{quote}
it's not a matter of taste, sometimes (depending on the bug) I do the same in projects that have already been released, not in an experiment. The bug is known and fixed in a previous version of the component, references in comments are fine but don't see the advantage of dedicated test in a new version.
alles gute!
> [BeanUtils2] Implement describe() on DefaultBeanAccessor
> ---------------------------------------------------------
>
> Key: SANDBOX-379
> URL: https://issues.apache.org/jira/browse/SANDBOX-379
> Project: Commons Sandbox
> Issue Type: Improvement
> Components: BeanUtils2
> Affects Versions: Nightly Builds
> Reporter: Benedikt Ritter
> Attachments: SANDBOX-379.txt
>
>
> Implement the above mentioned method an corresponding unit tests
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (SANDBOX-379) [BeanUtils2] Implement describe()
on DefaultBeanAccessor
Posted by "Simone Tripodi (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SANDBOX-379?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13198666#comment-13198666 ]
Simone Tripodi commented on SANDBOX-379:
----------------------------------------
Just terminated to have a look at the patch, good, few observations before applying it:
* {{BeanAccessorTestCase}} has only 1 test method that can be included in {{BeanUtilsTest}}; the {{describes}} is {{protected}} (and there's no reason why), it can be {{private final}}.
* Same thing for {{Jira157TestCase}}, test methods can be moved in the {{BeanUtilsTest}} class; methods name have to be renamed stripping the {{testIssue_BEANUTILS_157_BeanUtils}} prefix;
* {{AccessibleObjectsRegistry#getMethodsRegistry()}} can be referenced statically in {{PropertyDescriptorsRegistry}};
* {{PropertyDescriptorsRegistry#makeMethodsAccessible}} returns the same {{PropertyDescriptor}} input instance, there's no reason that method has a return statement, signature can be changed to {{void}}.
Please modify the patch so I can apply it
> [BeanUtils2] Implement describe() on DefaultBeanAccessor
> ---------------------------------------------------------
>
> Key: SANDBOX-379
> URL: https://issues.apache.org/jira/browse/SANDBOX-379
> Project: Commons Sandbox
> Issue Type: Improvement
> Components: BeanUtils2
> Affects Versions: Nightly Builds
> Reporter: Benedikt Ritter
> Attachments: SANDBOX-379.txt
>
>
> Implement the above mentioned method an corresponding unit tests
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (SANDBOX-379) [BeanUtils2] Implement describe()
on DefaultBeanAccessor
Posted by "Simone Tripodi (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SANDBOX-379?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13199626#comment-13199626 ]
Simone Tripodi commented on SANDBOX-379:
----------------------------------------
{quote}
As I said above, {{describe()}}, {{populate()}}, {{clone()}}, etc are the endpoints of a fluent call. So I thought putting them together in one test is a good idea.
{quote}
I don't, each method could involve a good number of test. See your contribution only for {{describe()}}.
{quote}
Okay, so do we create a test case for every method on {{BeanAccessor}}?
{quote}
Isn't what we are already doing? You already provided tests according to this...
{quote}
I think that would be a good idea. I don't like huge test classes. And a test class for all methods on {{BeanAccessor}} would be gigantic (as you mentioned).
{quote}
At least we have an agreement, looking forward your patch!
> [BeanUtils2] Implement describe() on DefaultBeanAccessor
> ---------------------------------------------------------
>
> Key: SANDBOX-379
> URL: https://issues.apache.org/jira/browse/SANDBOX-379
> Project: Commons Sandbox
> Issue Type: Improvement
> Components: BeanUtils2
> Affects Versions: Nightly Builds
> Reporter: Benedikt Ritter
> Attachments: SANDBOX-379.txt
>
>
> Implement the above mentioned method an corresponding unit tests
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira