kfaraz commented on a change in pull request #11561:
URL: https://github.com/apache/druid/pull/11561#discussion_r725673559
##########
File path:
processing/src/test/java/org/apache/druid/query/expression/MacroTestBase.java
##########
@@ -58,40 +63,36 @@ protected Expr apply(final List<Expr> args)
/**
* Evalutes {@code expr} using our macro.
*
- * @param expression expression to evalute
- * @param bindings bindings for evaluation
+ * @param expression
+ * expression to evalute
+ * @param bindings
+ * bindings for evaluation
*
- * @throws AssertionError if {@link ExprMacroTable.ExprMacro#apply} is not
called on our macro during parsing
+ * @throws AssertionError
+ * if {@link ExprMacroTable.ExprMacro#apply} is not called on our
+ * macro during parsing
*/
protected ExprEval<?> eval(
final String expression,
- final Expr.ObjectBinding bindings
- )
+ final Expr.ObjectBinding bindings)
{
- // WrappedExprMacro allows us to confirm that our ExprMacro was actually
called.
- class WrappedExprMacro implements ExprMacroTable.ExprMacro
- {
- private final AtomicLong calls = new AtomicLong();
-
- @Override
- public String name()
- {
- return macro.name();
- }
-
- @Override
- public Expr apply(List<Expr> args)
- {
- calls.incrementAndGet();
- return macro.apply(args);
- }
- }
-
- final WrappedExprMacro wrappedMacro = new WrappedExprMacro();
+ // Create variables for tracking behaviors of mock object
+ AtomicLong wrappedMacroCalls = new AtomicLong();
+ // Construct mock object
+ final ExprMacro wrappedMacro = mock(ExprMacroTable.ExprMacro.class);
+ // Method Stubs
+ when(wrappedMacro.name()).thenAnswer((stubInvo) -> {
+ return macro.name();
+ });
+ when(wrappedMacro.apply(any(List.class))).thenAnswer((stubInvo) -> {
+ List<Expr> args = stubInvo.getArgument(0);
+ wrappedMacroCalls.incrementAndGet();
Review comment:
Instead of using the AtomicLong counter `wrappedMacroCalls`, try using
`Mockito.verify()` with `Mockito.times()`.
##########
File path:
processing/src/test/java/org/apache/druid/query/expression/MacroTestBase.java
##########
@@ -58,40 +63,36 @@ protected Expr apply(final List<Expr> args)
/**
* Evalutes {@code expr} using our macro.
*
- * @param expression expression to evalute
- * @param bindings bindings for evaluation
+ * @param expression
+ * expression to evalute
+ * @param bindings
+ * bindings for evaluation
*
- * @throws AssertionError if {@link ExprMacroTable.ExprMacro#apply} is not
called on our macro during parsing
+ * @throws AssertionError
+ * if {@link ExprMacroTable.ExprMacro#apply} is not called on our
+ * macro during parsing
*/
protected ExprEval<?> eval(
final String expression,
- final Expr.ObjectBinding bindings
- )
+ final Expr.ObjectBinding bindings)
{
- // WrappedExprMacro allows us to confirm that our ExprMacro was actually
called.
- class WrappedExprMacro implements ExprMacroTable.ExprMacro
- {
- private final AtomicLong calls = new AtomicLong();
-
- @Override
- public String name()
- {
- return macro.name();
- }
-
- @Override
- public Expr apply(List<Expr> args)
- {
- calls.incrementAndGet();
- return macro.apply(args);
- }
- }
-
- final WrappedExprMacro wrappedMacro = new WrappedExprMacro();
+ // Create variables for tracking behaviors of mock object
+ AtomicLong wrappedMacroCalls = new AtomicLong();
+ // Construct mock object
+ final ExprMacro wrappedMacro = mock(ExprMacroTable.ExprMacro.class);
Review comment:
Please use one method of qualifying the class here, either
`ExprMacroTable.ExprMacro` or just `ExprMacro` (unless they are referring to
two different classes).
##########
File path:
processing/src/test/java/org/apache/druid/query/expression/MacroTestBase.java
##########
@@ -58,40 +63,36 @@ protected Expr apply(final List<Expr> args)
/**
* Evalutes {@code expr} using our macro.
*
- * @param expression expression to evalute
- * @param bindings bindings for evaluation
+ * @param expression
+ * expression to evalute
+ * @param bindings
+ * bindings for evaluation
*
- * @throws AssertionError if {@link ExprMacroTable.ExprMacro#apply} is not
called on our macro during parsing
+ * @throws AssertionError
+ * if {@link ExprMacroTable.ExprMacro#apply} is not called on our
+ * macro during parsing
*/
protected ExprEval<?> eval(
final String expression,
- final Expr.ObjectBinding bindings
- )
+ final Expr.ObjectBinding bindings)
{
- // WrappedExprMacro allows us to confirm that our ExprMacro was actually
called.
- class WrappedExprMacro implements ExprMacroTable.ExprMacro
- {
- private final AtomicLong calls = new AtomicLong();
-
- @Override
- public String name()
- {
- return macro.name();
- }
-
- @Override
- public Expr apply(List<Expr> args)
- {
- calls.incrementAndGet();
- return macro.apply(args);
- }
- }
-
- final WrappedExprMacro wrappedMacro = new WrappedExprMacro();
+ // Create variables for tracking behaviors of mock object
+ AtomicLong wrappedMacroCalls = new AtomicLong();
+ // Construct mock object
+ final ExprMacro wrappedMacro = mock(ExprMacroTable.ExprMacro.class);
Review comment:
Also, instead of wrapping the macro, I would advise just using
`Mockito.spy()` as you are essentially trying to do the same thing.
##########
File path:
processing/src/test/java/org/apache/druid/query/expression/MacroTestBase.java
##########
@@ -58,40 +63,36 @@ protected Expr apply(final List<Expr> args)
/**
* Evalutes {@code expr} using our macro.
*
- * @param expression expression to evalute
- * @param bindings bindings for evaluation
+ * @param expression
+ * expression to evalute
+ * @param bindings
+ * bindings for evaluation
*
- * @throws AssertionError if {@link ExprMacroTable.ExprMacro#apply} is not
called on our macro during parsing
+ * @throws AssertionError
+ * if {@link ExprMacroTable.ExprMacro#apply} is not called on our
+ * macro during parsing
*/
protected ExprEval<?> eval(
final String expression,
- final Expr.ObjectBinding bindings
- )
+ final Expr.ObjectBinding bindings)
Review comment:
Please remove these code/doc style changes unless you are
adding/removing content. This style does not align with the current style in
the rest of the code either.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
