You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2021/09/19 17:25:24 UTC
[GitHub] [avro] wx930910 opened a new pull request #1341: refactor printinVisitor to improve testing logic
wx930910 opened a new pull request #1341:
URL: https://github.com/apache/avro/pull/1341
### Jira
- AVRO-3211: Refactor PrintingVisitor to improve test design
### Description
#### Replace test class [PrintingVisitor](https://github.com/apache/avro/blob/42822886c28ea74a744abb7e7a80a942c540faa5/lang/java/compiler/src/test/java/org/apache/avro/compiler/schema/TestSchemas.java#L41) by mocking object and improve test design
<hr>
##### Motivation
- Decouple test class `PrintingVisitor` from production interface `SchemaVisitor`.
- Remove the redundant test child class `PrintingVisitor`.
<hr>
##### Key changed/added classes in this PR
* Created mocking object to replace test subclass `PrintingVisitor`, decoupled test from production code.
* Created method that return the mocking object for code reuse.
* Make test logic more clear by using method stub instead of method overriding.
* Remove redundant overridden method `get()`.
* Add Mockito dependency.
<hr>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] wx930910 commented on pull request #1341: refactor printinVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
wx930910 commented on pull request #1341:
URL: https://github.com/apache/avro/pull/1341#issuecomment-922508284
In addition to this change, I think if we only use PrintingVisitor to log out information, we can actually get rid of `PrintingVisitor` and spy `CloningVisitor`, then use `Mockito.verify()` to verify method execution times and `Mockito.inOrder()` to verify the method execution order based on the given Schema. We can make test condition more clear and empower test case by verify execution order explictly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a change in pull request #1341: refactor printinVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1341:
URL: https://github.com/apache/avro/pull/1341#discussion_r711794238
##########
File path: lang/java/pom.xml
##########
@@ -65,6 +65,7 @@
<exec-plugin.version>3.0.0</exec-plugin.version>
<file-management.version>3.0.0</file-management.version>
<javacc-plugin.version>2.6</javacc-plugin.version>
+ <mockito.version>3.9.0</mockito.version>
Review comment:
Do you say that there are some regressions in the newer versions ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] wx930910 commented on a change in pull request #1341: refactor printinVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
wx930910 commented on a change in pull request #1341:
URL: https://github.com/apache/avro/pull/1341#discussion_r711793508
##########
File path: lang/java/compiler/pom.xml
##########
@@ -228,6 +228,11 @@
<!-- can only be used from within ant -->
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <version>${mockito.version}</version>
Review comment:
Yes, I'll limit the Mockito within test scope.
##########
File path: lang/java/compiler/src/test/java/org/apache/avro/compiler/schema/TestSchemas.java
##########
@@ -38,36 +59,11 @@
+ " {\"name\":\"methodName\",\"type\":{\"type\":\"string\",\"avro.java.string\":\"String\"}}\n"
+ " ]}},\n" + " {\"name\":\"node\",\"type\":\"SampleNode\"}]}}}" + "]}";
- private static class PrintingVisitor implements SchemaVisitor {
-
- @Override
- public SchemaVisitorAction visitTerminal(Schema terminal) {
- System.out.println("Terminal: " + terminal.getFullName());
- return SchemaVisitorAction.CONTINUE;
- }
-
- @Override
- public SchemaVisitorAction visitNonTerminal(Schema terminal) {
- System.out.println("NONTerminal start: " + terminal.getFullName());
- return SchemaVisitorAction.CONTINUE;
- }
-
- @Override
- public SchemaVisitorAction afterVisitNonTerminal(Schema terminal) {
- System.out.println("NONTerminal end: " + terminal.getFullName());
- return SchemaVisitorAction.CONTINUE;
- }
-
- @Override
- public Object get() {
- return null;
- }
Review comment:
Yes, actually if we only want to confirm the execution order of the visitor, we can just spy on `CloningVisitor` and totaly get rid of the `printingVisitor`, do you think that will be better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] wx930910 commented on a change in pull request #1341: refactor printinVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
wx930910 commented on a change in pull request #1341:
URL: https://github.com/apache/avro/pull/1341#discussion_r711794634
##########
File path: lang/java/pom.xml
##########
@@ -65,6 +65,7 @@
<exec-plugin.version>3.0.0</exec-plugin.version>
<file-management.version>3.0.0</file-management.version>
<javacc-plugin.version>2.6</javacc-plugin.version>
+ <mockito.version>3.9.0</mockito.version>
Review comment:
Not sure if there are regressions in the newer version, but according to [Maven Repository](https://mvnrepository.com/artifact/org.mockito/mockito-core), 3.9.0 is the most well accepted version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a change in pull request #1341: refactor printinVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1341:
URL: https://github.com/apache/avro/pull/1341#discussion_r711792595
##########
File path: lang/java/compiler/pom.xml
##########
@@ -228,6 +228,11 @@
<!-- can only be used from within ant -->
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <version>${mockito.version}</version>
Review comment:
Does it need `<scope>test</scope>`? I didn't find a dependencyManagement in the parent poms for it.
##########
File path: lang/java/compiler/src/test/java/org/apache/avro/compiler/schema/TestSchemas.java
##########
@@ -38,36 +59,11 @@
+ " {\"name\":\"methodName\",\"type\":{\"type\":\"string\",\"avro.java.string\":\"String\"}}\n"
+ " ]}},\n" + " {\"name\":\"node\",\"type\":\"SampleNode\"}]}}}" + "]}";
- private static class PrintingVisitor implements SchemaVisitor {
-
- @Override
- public SchemaVisitorAction visitTerminal(Schema terminal) {
- System.out.println("Terminal: " + terminal.getFullName());
- return SchemaVisitorAction.CONTINUE;
- }
-
- @Override
- public SchemaVisitorAction visitNonTerminal(Schema terminal) {
- System.out.println("NONTerminal start: " + terminal.getFullName());
- return SchemaVisitorAction.CONTINUE;
- }
-
- @Override
- public SchemaVisitorAction afterVisitNonTerminal(Schema terminal) {
- System.out.println("NONTerminal end: " + terminal.getFullName());
- return SchemaVisitorAction.CONTINUE;
- }
-
- @Override
- public Object get() {
- return null;
- }
Review comment:
I don't see much value in introducing Mockito for this. This visitor is very simple! It looks even simpler than the new Mockito based one.
##########
File path: lang/java/pom.xml
##########
@@ -65,6 +65,7 @@
<exec-plugin.version>3.0.0</exec-plugin.version>
<file-management.version>3.0.0</file-management.version>
<javacc-plugin.version>2.6</javacc-plugin.version>
+ <mockito.version>3.9.0</mockito.version>
Review comment:
Why 3.9.0 ?
Current latest is 3.12.4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba merged pull request #1341: AVRO-3211: Refactor PrintingVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
RyanSkraba merged pull request #1341:
URL: https://github.com/apache/avro/pull/1341
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] wx930910 commented on a change in pull request #1341: refactor printinVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
wx930910 commented on a change in pull request #1341:
URL: https://github.com/apache/avro/pull/1341#discussion_r711798004
##########
File path: lang/java/compiler/src/test/java/org/apache/avro/compiler/schema/TestSchemas.java
##########
@@ -38,36 +59,11 @@
+ " {\"name\":\"methodName\",\"type\":{\"type\":\"string\",\"avro.java.string\":\"String\"}}\n"
+ " ]}},\n" + " {\"name\":\"node\",\"type\":\"SampleNode\"}]}}}" + "]}";
- private static class PrintingVisitor implements SchemaVisitor {
-
- @Override
- public SchemaVisitorAction visitTerminal(Schema terminal) {
- System.out.println("Terminal: " + terminal.getFullName());
- return SchemaVisitorAction.CONTINUE;
- }
-
- @Override
- public SchemaVisitorAction visitNonTerminal(Schema terminal) {
- System.out.println("NONTerminal start: " + terminal.getFullName());
- return SchemaVisitorAction.CONTINUE;
- }
-
- @Override
- public SchemaVisitorAction afterVisitNonTerminal(Schema terminal) {
- System.out.println("NONTerminal end: " + terminal.getFullName());
- return SchemaVisitorAction.CONTINUE;
- }
-
- @Override
- public Object get() {
- return null;
- }
Review comment:
Yes I agree with you. But just adding the print info doesn't effect the testing result, won't it better to explictly verify the execution times and order to enpower the test? Do you think the verification of execution order seems redundant in this case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #1341: AVRO-3211: Refactor PrintingVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #1341:
URL: https://github.com/apache/avro/pull/1341#issuecomment-925750713
> What do you think about just dropping PrintingVisitor entirely?
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] wx930910 commented on pull request #1341: AVRO-3211: Refactor PrintingVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
wx930910 commented on pull request #1341:
URL: https://github.com/apache/avro/pull/1341#issuecomment-927341283
> What do you think about just dropping PrintingVisitor entirely?
I agree with you! Changed the PR and dropped the PrintingVisitor entirely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] wx930910 edited a comment on pull request #1341: AVRO-3211: Refactor PrintingVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
wx930910 edited a comment on pull request #1341:
URL: https://github.com/apache/avro/pull/1341#issuecomment-927341283
> What do you think about just dropping PrintingVisitor entirely?
I agree with you! Changed the PR and dropped the PrintingVisitor entirely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] wx930910 commented on a change in pull request #1341: refactor printinVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
wx930910 commented on a change in pull request #1341:
URL: https://github.com/apache/avro/pull/1341#discussion_r711793601
##########
File path: lang/java/pom.xml
##########
@@ -65,6 +65,7 @@
<exec-plugin.version>3.0.0</exec-plugin.version>
<file-management.version>3.0.0</file-management.version>
<javacc-plugin.version>2.6</javacc-plugin.version>
+ <mockito.version>3.9.0</mockito.version>
Review comment:
That's because 3.9.0 is the most widly used/stable version of Mockito
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a change in pull request #1341: refactor printinVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1341:
URL: https://github.com/apache/avro/pull/1341#discussion_r711794865
##########
File path: lang/java/compiler/src/test/java/org/apache/avro/compiler/schema/TestSchemas.java
##########
@@ -38,36 +59,11 @@
+ " {\"name\":\"methodName\",\"type\":{\"type\":\"string\",\"avro.java.string\":\"String\"}}\n"
+ " ]}},\n" + " {\"name\":\"node\",\"type\":\"SampleNode\"}]}}}" + "]}";
- private static class PrintingVisitor implements SchemaVisitor {
-
- @Override
- public SchemaVisitorAction visitTerminal(Schema terminal) {
- System.out.println("Terminal: " + terminal.getFullName());
- return SchemaVisitorAction.CONTINUE;
- }
-
- @Override
- public SchemaVisitorAction visitNonTerminal(Schema terminal) {
- System.out.println("NONTerminal start: " + terminal.getFullName());
- return SchemaVisitorAction.CONTINUE;
- }
-
- @Override
- public SchemaVisitorAction afterVisitNonTerminal(Schema terminal) {
- System.out.println("NONTerminal end: " + terminal.getFullName());
- return SchemaVisitorAction.CONTINUE;
- }
-
- @Override
- public Object get() {
- return null;
- }
Review comment:
One could also override the methods, print and call super:
```
CloningVisitor cv = new CloningVisitor(recSchema) {
@Override
public SchemaVisitorAction visitTerminal(Schema terminal) {
System.out.println("Terminal: " + terminal.getFullName());
return super.visitTerminal(terminal);
}
// ...
};
```
But if you need to do this at many places then it becomes ugly. Having around `PrintingVisitor` make is as easy as `Schemas.visit(recSchema, new PrintingVisitor());`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] wx930910 commented on a change in pull request #1341: refactor printinVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
wx930910 commented on a change in pull request #1341:
URL: https://github.com/apache/avro/pull/1341#discussion_r711795716
##########
File path: lang/java/pom.xml
##########
@@ -65,6 +65,7 @@
<exec-plugin.version>3.0.0</exec-plugin.version>
<file-management.version>3.0.0</file-management.version>
<javacc-plugin.version>2.6</javacc-plugin.version>
+ <mockito.version>3.9.0</mockito.version>
Review comment:
I see, thanks for the advice!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] wx930910 edited a comment on pull request #1341: refactor printinVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
wx930910 edited a comment on pull request #1341:
URL: https://github.com/apache/avro/pull/1341#issuecomment-922508284
In addition to this change, I think if we only use `PrintingVisitor` to log out information, we can actually get rid of `PrintingVisitor` and spy `CloningVisitor`, then use `Mockito.verify()` to verify method execution times and `Mockito.inOrder()` to verify the method execution order based on the given Schema. We can make test condition more clear and empower test case by verify execution order explictly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba merged pull request #1341: AVRO-3211: Refactor PrintingVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
RyanSkraba merged pull request #1341:
URL: https://github.com/apache/avro/pull/1341
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a change in pull request #1341: refactor printinVisitor to improve testing logic
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1341:
URL: https://github.com/apache/avro/pull/1341#discussion_r711795165
##########
File path: lang/java/pom.xml
##########
@@ -65,6 +65,7 @@
<exec-plugin.version>3.0.0</exec-plugin.version>
<file-management.version>3.0.0</file-management.version>
<javacc-plugin.version>2.6</javacc-plugin.version>
+ <mockito.version>3.9.0</mockito.version>
Review comment:
3.0.0 has even more usages than 3.9.0 :-)
I hope you understand that this metric should not lead you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org