Skip to content

remove unused code of TestJavassistUtils and make it readable #337

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 30, 2017
Merged

remove unused code of TestJavassistUtils and make it readable #337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
152b95d
delete unused method in the test
wldandan Nov 30, 2017
10b6607
Add the rule to ignore IDEA project files
wldandan Nov 30, 2017
8ddee43
clean the format and remove unused code
wldandan Nov 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ __pycache__/
# IntelliJ, based on http://devnet.jetbrains.net/docs/DOC-1186
.idea/
*.iml
*.ipr
*.iws

# logs and trace
*.log
Expand Down
13 changes: 2 additions & 11 deletions ...on/common-javassist/src/test/java/io/servicecomb/common/javassist/TestJavassistUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,29 +54,23 @@ public void testField() throws Exception {

Class<?> cls = JavassistUtils.createClass(classConfig);

// intField
Field field = cls.getField("intField");
Assert.assertEquals(Integer.class, field.getType());

Method method = cls.getMethod("getIntField");
Assert.assertEquals(Integer.class, method.getReturnType());
method = cls.getMethod("setIntField", Integer.class);

// intArrayField
field = cls.getField("intArrayField");
Assert.assertEquals(int[].class, field.getType());

method = cls.getMethod("getIntArrayField");
Assert.assertEquals(int[].class, method.getReturnType());
method = cls.getMethod("setIntArrayField", int[].class);

// listStringField
field = cls.getField("listStringField");
Assert.assertEquals("java.util.List<java.lang.String>", field.getGenericType().getTypeName());

method = cls.getMethod("getListStringField");
Assert.assertEquals("java.util.List<java.lang.String>", method.getGenericReturnType().getTypeName());
method = cls.getMethod("setListStringField", List.class);
Copy link
Contributor

@wujimin wujimin Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleted "getMethod" is to ensure setter is exist and has correct signature
if has any problems, will throw exception.

Copy link
Member

@WillemJiang WillemJiang Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to use assert the verify the call result.

Copy link
Contributor Author

@wldandan wldandan Nov 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If "getMethod" has logic as you mentioned, I think it should be verified rather than just let it be a caller. For example, we can add the following assert for the logic if we really care about it :

method = cls.getMethod("setListStringField", List.class);
assertNotNull(method);

Copy link
Member

@WillemJiang WillemJiang Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wldandan Please add a new commit for it.

Copy link
Contributor

@wujimin wujimin Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

infact, if cls.getMethod return null, will throw exception inside getMethod
so the assert is invalid.

Copy link
Contributor

@wujimin wujimin Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you care this so much, you can invoke org.springframework.util.ReflectionUtils.findMethod(Class, String, Class...)
then if method not exists, will get a null result
but this method invoked getDeclaredMethod inside it, not invoked getMethod, so after assert null, you must assert if it's public.

Copy link
Contributor Author

@wldandan wldandan Dec 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. my thought is straightforward. From my point of view, the test case is best entrance for beginner to get familiar with the functionality.

If some methods are called in test case, it means that we expect it to be verified.

Back to the discussion,
As you mentioned, "deleted "getMethod" is to ensure setter is exist and has correct signature if has any problems, will throw exception."

The logic is implicit, I absolutely think it should be verified if we call the method.
For the case, what is your suggestion if we want to verify it?

method = cls.getMethod("setListStringField", List.class);

Copy link
Member

@WillemJiang WillemJiang Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to verify the method, if the getMethod throw the exception JUnit can still know about and tell us something is wrong. As a client we want to make sure is the method is not null and we can still use it. If we want to verify the non-exist method, we also need to add expected exception into JUnit test case to make sure the exception is thrown.

Copy link
Contributor Author

@wldandan wldandan Dec 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WillemJiang +1. To keep the scope limited, I think we can close the loop and add another PR to verify "getmethod" for the scenario about throwing exception.
I will do that soon.
Thanks guys.

}

@Test
Expand All @@ -86,7 +80,6 @@ public void testAddParameter() {
String intfName = "cse.ut.TestAddParameter";
classConfig.setClassName(intfName);

// List<String> method(Map<String, String> map, Set<String> set)
Copy link
Contributor

@wujimin wujimin Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comments is what the target method will be.

Copy link
Contributor Author

@wldandan wldandan Nov 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is easy to implement that if we need it in one day, and I do not think it is worth to highlighting it just for recall someone that the method will be in future.

Copy link
Contributor

@wujimin wujimin Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, we create a new class in memory, and add a new method in the class
if we did not comments the targt method signature, it's hard to know what's the purpose

so i thinks, comments can be better, but delete it is not good.

Copy link
Contributor Author

@wldandan wldandan Dec 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it reasonable if we define another method to hold the structure or use test case to clarify it?
Or should we add more detailed comments to mention that rather than just a method definition?

MethodConfig methodConfig = new MethodConfig();
methodConfig.setName("method");
methodConfig.setResult(TypeFactory.defaultInstance().constructCollectionType(List.class, String.class));
Expand Down Expand Up @@ -198,10 +191,8 @@ public void testGetNameForGenerateCode() {

@Test
public void managerClassPool() {
ClassLoader classLoader1 = new ClassLoader() {
};
ClassLoader classLoader2 = new ClassLoader() {
};
ClassLoader classLoader1 = new ClassLoader() { };
Copy link
Contributor

@wujimin wujimin Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after format code, it will rollback to be the deleted style.

Copy link
Contributor Author

@wldandan wldandan Nov 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.
It might not be an issue. However, to make it human readable, I suggest that removing the line break if there is nothing inside.

Copy link
Contributor

@wujimin wujimin Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree.
but it seems that hard to write this rule both in IntelliJ and eclipse

Copy link
Contributor Author

@wldandan wldandan Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave it as an issue and put it in the queue. :)

ClassLoader classLoader2 = new ClassLoader() { };

ClassPool p1 = Deencapsulation.invoke(JavassistUtils.class, "getOrCreateClassPool", classLoader1);
ClassPool p2 = Deencapsulation.invoke(JavassistUtils.class, "getOrCreateClassPool", classLoader2);
Expand Down