Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can we simplify use of @Param annotation #13

Open
ravi-polampelli opened this issue Aug 30, 2012 · 11 comments
Open

Can we simplify use of @Param annotation #13

ravi-polampelli opened this issue Aug 30, 2012 · 11 comments
Assignees
Labels

Comments

@ravi-polampelli
Copy link

Hi Anuj,

Q1: For simplicity, can we remove @param annotation declaration from test methods?
Reasons :

  1. I see it is used for Parameter name, Param name is implicit, if we make it clear up front that field name in file should match exactly same as parameter.
  2. It is implicit that parameters are passed.
  3. Method signature looks neat and simplified,
    public void testFindItemsWithReturnType(@param()
    LibraryId id , @param(name="itemid") ItemId itemId) { ..}
    Can be changed to
    public void testFindItemsWithReturnType(LibraryId id , ItemId itemId) { .. }
    Note : we can keep Data Supplier static class separately.

Q2: Can we wrap primitive types(int, long..) and simple types (Long, Double, Boolean..) with out need of user registering property editors?
If we know that declared parameter is Long, then we can convert String from file to Long.
If we can bring this change then user will be relaxed in writing test methods for these Data types and Test method will look more simple and intutive.
For ex: public void testFindItemsWithReturnType(Long libraryId , Long itemId) { .. }
Note: we can use converters anyhow for complex types / Objects.

I am not sure if there are any implications for these changes, these are just ideas from end user point of view and intuitive usability objective. Let me know your thoughts..

Cheers,
Ravi

@anujgandharv
Copy link
Member

Hi Ravi,
There are certain implications here in the way @param annotation works. It is using quite a functionality from JUnit. To be honest, it is a good idea and we can keep a track of it by opening enhancement issues, but I will keep them in the back log for now. We do not even have a core of EasyTest developed. I first want the core functionality to be in place before we start refactoring our code. I say this because of many reasons:

  1. I am slated to talk in the Java User Group about EasyTest and its capabilities in the end of October. Therefore I want the core functionality in place before that so that it leaves a strong impression.
  2. We now have a repository assigned to us in Maven and can now start deploying our artifacts for the outer world to easily consume. But I still cannot promote it because we do not have a Excel and XML based data loader in place.

Thus I want to bring back the focus on core development and handling enhancement issues later.

Cheers!

@anujgandharv
Copy link
Member

Hi Ravi, I feel I might have not conveyed my thoughts correctly by the above comment and it might have sounded a bit rude. It was not my intention. What I was trying to say was if you feel strongly about making @param annotation optional, then by all means go ahead with it. The project will only benefit from that change. I personally think that it might not be a small change though and you have to delve a little deeper in the code base. A good starting point for you could be the following line inside EasyTestRunner's runWithAssignment method:

parameterAssignment.potentialsForNextUnassigned()

This is the method that I think is responsible for looking at ParameterSuppliedBy and Param annotations and get the method arguments back.
Do let me know if you want to pursue this further and I will remove it from the back log.

Cheers!

@ravi-polampelli
Copy link
Author

Don't worry Mate, nothing rude in that. I see its going good and we have same wavelength.
I think this @param change can be include in first version as it becomes part of core, other things looks good.
I can spend time in excel stuff.
Regarding @param change, i will look into the code again and come back with implication points, at initial look I felt Data Supplier static class need to take out from @param annotation and keep it separately.

Cheers,
Ravi

@anujgandharv
Copy link
Member

Ravi, Excel loader is now in place and available in Github. Can yuou please review it and see if there's anything missing or not correct.

Also XML Loader will be in by end of day today.

If you want you can start looking at making @param optional. Dont forget to assign the issue to you before starting so that we know whose working on what.

Cheers!

@ghost ghost assigned ravi-polampelli Aug 31, 2012
@ravi-polampelli
Copy link
Author

It looks not possible to remove @param as it is tied to ParameterSuppliedBy, it is being used internally by JUnit.
I thought at least we can avoid name variable in Param annotation and try to use the parameter name given in the test case. But even that looks not possible as ParameterSignature takes only data type and annotations. Ummh..so no more simplification possible.
Please let me know your views.

@anujgandharv
Copy link
Member

Lets close this one and may be in future we can rewrite the whole @param implementation such that it becomes optional and does not depend on ParameterSuppliedBy annotation.

@anujgandharv
Copy link
Member

Hey Ravi,
I have made @param annotation now optional.
A user can now have a test method like this: public void testMethod(LibraryId libId, itemId itemId);

We cannot make it go away because then user's Java code has to be compiled with debug enabled option which we cannot gaurantee. Thus, in case user has more than one parameter of the same type then he has to provide the @param annotation with name attribute for one of the parameters.
Moreover. the name attribute will now become mandatory instead of optional. Since in case it is optional, user can omit the @param annotation alltogether.

@anujgandharv anujgandharv reopened this Nov 12, 2012
@ravi-polampelli
Copy link
Author

Sounds good. I will try once and let you know the feedback

On Mon, Nov 12, 2012 at 4:28 PM, Anuj Kumar [email protected]:

Hey Ravi,
I have made @param https://github.com/Param annotation now optional.
A user can now have a test method like this: public void
testMethod(LibraryId libId, itemId itemId);

We cannot make it go away because then user's Java code has to be comiled
with debug enabled option which we cannot gaurantee. Thus in case user has
more than one parameter of the same type then he has to provide the @Paramhttps://github.com/Paramannotation with name attribute for one of the parameters.
Moreover. the name attribute will now become mandatory instead of
optional. Since in case it is optional, user can omit the @Paramhttps://github.com/Paramannotation alltogether.


Reply to this email directly or view it on GitHubhttps://github.com//issues/13#issuecomment-10293994.

@anujgandharv
Copy link
Member

I still have it on my local. Will check in soon.

@ravi-polampelli
Copy link
Author

I have a doubt.
As you said, incase user has more than one parameter of the same type then he has to provide the @param annotation. It looks to me that it might create confusion to user that sometimes he has to create @param annotation and some times not. So there is a chance he might miss giving the annotation for same data type, this will lead incorrect mapping of test data and incorrect results.
Can we add validation in our code if he misses @param annotation for same data type?

@anujgandharv
Copy link
Member

Hi Ravi,
I think its an excellent idea to have validation in our code if the user misses @param annotation for the data type that occurs more than once in the test case as input parameter.

I will have a look at how we can gracefully handle such a scenario.
Will create a new Issue in EasyTest Core for tracking this.

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants