You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Mike Drob <md...@mdrob.com> on 2014/03/20 00:27:38 UTC
Review Request 19428: ACCUMULO-2503 add formatter tests
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19428/
-----------------------------------------------------------
Review request for accumulo.
Bugs: ACCUMULO-2503
https://issues.apache.org/jira/browse/ACCUMULO-2503
Repository: accumulo
Description
-------
ACCUMULO-2503 add formatter tests
Diffs
-----
core/src/main/java/org/apache/accumulo/core/util/format/BinaryFormatter.java 5021d66fc9117ff97203f6a46ec7422b9db3a598
core/src/main/java/org/apache/accumulo/core/util/format/DefaultFormatter.java ee4a2200a433d2e5a0268066421426af4594d346
core/src/main/java/org/apache/accumulo/core/util/format/DeleterFormatter.java 8547f12e4c7f7d2435cf890be37f5fd9279dcb63
core/src/main/java/org/apache/accumulo/core/util/format/FormatterFactory.java 5451843b114ff8d605511ce7dd21850c4048e893
core/src/main/java/org/apache/accumulo/core/util/format/HexFormatter.java 1def7127a58c73313618c739bdd7f50bb8bc772f
core/src/main/java/org/apache/accumulo/core/util/format/ShardedTableDistributionFormatter.java 577167a150b8861de714fdde79d2ba41ed06fb89
core/src/main/java/org/apache/accumulo/core/util/format/StatisticsDisplayFormatter.java dd9de6ce6b967210322bec6c9b704f46b6031917
core/src/test/java/org/apache/accumulo/core/util/format/DateStringFormatterTest.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/format/DefaultFormatterTest.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/format/DeleterFormatterTest.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/format/FormatterFactoryTest.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/format/HexFormatterTest.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/format/ShardedTableDistributionFormatterTest.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/format/StatisticsDisplayFormatterTest.java PRE-CREATION
core/src/test/resources/log4j.properties dfc93bf2f1bf04de85e5dd8fca1b966dc803e067
Diff: https://reviews.apache.org/r/19428/diff/
Testing
-------
New and old unit tests pass.
Thanks,
Mike Drob
Re: Review Request 19428: ACCUMULO-2503 add formatter tests
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19428/#review38435
-----------------------------------------------------------
Ship it!
Ship It!
- Bill Havanki
On March 24, 2014, 12:01 p.m., Mike Drob wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19428/
> -----------------------------------------------------------
>
> (Updated March 24, 2014, 12:01 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2503
> https://issues.apache.org/jira/browse/ACCUMULO-2503
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> ACCUMULO-2503 add formatter tests
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/format/AggregatingFormatter.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/util/format/BinaryFormatter.java 5021d66fc9117ff97203f6a46ec7422b9db3a598
> core/src/main/java/org/apache/accumulo/core/util/format/DefaultFormatter.java ee4a2200a433d2e5a0268066421426af4594d346
> core/src/main/java/org/apache/accumulo/core/util/format/DeleterFormatter.java 8547f12e4c7f7d2435cf890be37f5fd9279dcb63
> core/src/main/java/org/apache/accumulo/core/util/format/FormatterFactory.java 5451843b114ff8d605511ce7dd21850c4048e893
> core/src/main/java/org/apache/accumulo/core/util/format/HexFormatter.java 1def7127a58c73313618c739bdd7f50bb8bc772f
> core/src/main/java/org/apache/accumulo/core/util/format/ShardedTableDistributionFormatter.java 577167a150b8861de714fdde79d2ba41ed06fb89
> core/src/main/java/org/apache/accumulo/core/util/format/StatisticsDisplayFormatter.java dd9de6ce6b967210322bec6c9b704f46b6031917
> core/src/test/java/org/apache/accumulo/core/util/format/DateStringFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/DefaultFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/DeleterFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/FormatterFactoryTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/HexFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/ShardedTableDistributionFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/StatisticsDisplayFormatterTest.java PRE-CREATION
> core/src/test/resources/log4j.properties dfc93bf2f1bf04de85e5dd8fca1b966dc803e067
>
> Diff: https://reviews.apache.org/r/19428/diff/
>
>
> Testing
> -------
>
> New and old unit tests pass.
>
>
> Thanks,
>
> Mike Drob
>
>
Re: Review Request 19428: ACCUMULO-2503 add formatter tests
Posted by Mike Drob <md...@mdrob.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19428/
-----------------------------------------------------------
(Updated March 24, 2014, 4:01 p.m.)
Review request for accumulo.
Changes
-------
Incorporated Bill H.'s suggestions. Switched empty values to use the new no-arg constructor.
Bugs: ACCUMULO-2503
https://issues.apache.org/jira/browse/ACCUMULO-2503
Repository: accumulo
Description
-------
ACCUMULO-2503 add formatter tests
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/util/format/AggregatingFormatter.java PRE-CREATION
core/src/main/java/org/apache/accumulo/core/util/format/BinaryFormatter.java 5021d66fc9117ff97203f6a46ec7422b9db3a598
core/src/main/java/org/apache/accumulo/core/util/format/DefaultFormatter.java ee4a2200a433d2e5a0268066421426af4594d346
core/src/main/java/org/apache/accumulo/core/util/format/DeleterFormatter.java 8547f12e4c7f7d2435cf890be37f5fd9279dcb63
core/src/main/java/org/apache/accumulo/core/util/format/FormatterFactory.java 5451843b114ff8d605511ce7dd21850c4048e893
core/src/main/java/org/apache/accumulo/core/util/format/HexFormatter.java 1def7127a58c73313618c739bdd7f50bb8bc772f
core/src/main/java/org/apache/accumulo/core/util/format/ShardedTableDistributionFormatter.java 577167a150b8861de714fdde79d2ba41ed06fb89
core/src/main/java/org/apache/accumulo/core/util/format/StatisticsDisplayFormatter.java dd9de6ce6b967210322bec6c9b704f46b6031917
core/src/test/java/org/apache/accumulo/core/util/format/DateStringFormatterTest.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/format/DefaultFormatterTest.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/format/DeleterFormatterTest.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/format/FormatterFactoryTest.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/format/HexFormatterTest.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/format/ShardedTableDistributionFormatterTest.java PRE-CREATION
core/src/test/java/org/apache/accumulo/core/util/format/StatisticsDisplayFormatterTest.java PRE-CREATION
core/src/test/resources/log4j.properties dfc93bf2f1bf04de85e5dd8fca1b966dc803e067
Diff: https://reviews.apache.org/r/19428/diff/
Testing
-------
New and old unit tests pass.
Thanks,
Mike Drob
Re: Review Request 19428: ACCUMULO-2503 add formatter tests
Posted by Mike Drob <md...@mdrob.com>.
> On March 20, 2014, 2:17 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/util/format/FormatterFactory.java, line 25
> > <https://reviews.apache.org/r/19428/diff/1/?file=528570#file528570line25>
> >
> > This would be a great candidate for conversion to a regular class instead of static only, but that may be out of scope for this effort.
+1 for out of scope.
> On March 20, 2014, 2:17 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/util/format/BinaryFormatter.java, line 136
> > <https://reviews.apache.org/r/19428/diff/1/?file=528567#file528567line136>
> >
> > This is the setter method that needs renaming, right?
Yes, but I think that's out of scope. Refactoring this whole class is a task onto itself.
> On March 20, 2014, 2:17 p.m., Bill Havanki wrote:
> > core/src/test/java/org/apache/accumulo/core/util/format/DefaultFormatterTest.java, line 36
> > <https://reviews.apache.org/r/19428/diff/1/?file=528575#file528575line36>
> >
> > Test suggestion: an empty byte array.
Done.
> On March 20, 2014, 2:17 p.m., Bill Havanki wrote:
> > core/src/test/java/org/apache/accumulo/core/util/format/DeleterFormatterTest.java, line 68
> > <https://reviews.apache.org/r/19428/diff/1/?file=528576#file528576line68>
> >
> > You don't need to replay a mock if you don't expect any calls on it, but are just using it as a stub to be passed around or returned.
Makes sense. Done.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19428/#review37887
-----------------------------------------------------------
On March 19, 2014, 11:27 p.m., Mike Drob wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19428/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 11:27 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2503
> https://issues.apache.org/jira/browse/ACCUMULO-2503
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> ACCUMULO-2503 add formatter tests
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/format/BinaryFormatter.java 5021d66fc9117ff97203f6a46ec7422b9db3a598
> core/src/main/java/org/apache/accumulo/core/util/format/DefaultFormatter.java ee4a2200a433d2e5a0268066421426af4594d346
> core/src/main/java/org/apache/accumulo/core/util/format/DeleterFormatter.java 8547f12e4c7f7d2435cf890be37f5fd9279dcb63
> core/src/main/java/org/apache/accumulo/core/util/format/FormatterFactory.java 5451843b114ff8d605511ce7dd21850c4048e893
> core/src/main/java/org/apache/accumulo/core/util/format/HexFormatter.java 1def7127a58c73313618c739bdd7f50bb8bc772f
> core/src/main/java/org/apache/accumulo/core/util/format/ShardedTableDistributionFormatter.java 577167a150b8861de714fdde79d2ba41ed06fb89
> core/src/main/java/org/apache/accumulo/core/util/format/StatisticsDisplayFormatter.java dd9de6ce6b967210322bec6c9b704f46b6031917
> core/src/test/java/org/apache/accumulo/core/util/format/DateStringFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/DefaultFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/DeleterFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/FormatterFactoryTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/HexFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/ShardedTableDistributionFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/StatisticsDisplayFormatterTest.java PRE-CREATION
> core/src/test/resources/log4j.properties dfc93bf2f1bf04de85e5dd8fca1b966dc803e067
>
> Diff: https://reviews.apache.org/r/19428/diff/
>
>
> Testing
> -------
>
> New and old unit tests pass.
>
>
> Thanks,
>
> Mike Drob
>
>
Re: Review Request 19428: ACCUMULO-2503 add formatter tests
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19428/#review37887
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/util/format/BinaryFormatter.java
<https://reviews.apache.org/r/19428/#comment69661>
This is the setter method that needs renaming, right?
core/src/main/java/org/apache/accumulo/core/util/format/FormatterFactory.java
<https://reviews.apache.org/r/19428/#comment69664>
This would be a great candidate for conversion to a regular class instead of static only, but that may be out of scope for this effort.
core/src/test/java/org/apache/accumulo/core/util/format/DefaultFormatterTest.java
<https://reviews.apache.org/r/19428/#comment69662>
Test suggestion: an empty byte array.
core/src/test/java/org/apache/accumulo/core/util/format/DeleterFormatterTest.java
<https://reviews.apache.org/r/19428/#comment69663>
You don't need to replay a mock if you don't expect any calls on it, but are just using it as a stub to be passed around or returned.
core/src/test/java/org/apache/accumulo/core/util/format/HexFormatterTest.java
<https://reviews.apache.org/r/19428/#comment69667>
Test suggestion: empty byte array.
core/src/test/java/org/apache/accumulo/core/util/format/HexFormatterTest.java
<https://reviews.apache.org/r/19428/#comment69666>
There should also be a half-trip test that ensures the hex conversion is correct, i.e., that a 9 byte becomes 9, a 10 byte becomes a, etc.
- Bill Havanki
On March 19, 2014, 7:27 p.m., Mike Drob wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19428/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 7:27 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2503
> https://issues.apache.org/jira/browse/ACCUMULO-2503
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> ACCUMULO-2503 add formatter tests
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/util/format/BinaryFormatter.java 5021d66fc9117ff97203f6a46ec7422b9db3a598
> core/src/main/java/org/apache/accumulo/core/util/format/DefaultFormatter.java ee4a2200a433d2e5a0268066421426af4594d346
> core/src/main/java/org/apache/accumulo/core/util/format/DeleterFormatter.java 8547f12e4c7f7d2435cf890be37f5fd9279dcb63
> core/src/main/java/org/apache/accumulo/core/util/format/FormatterFactory.java 5451843b114ff8d605511ce7dd21850c4048e893
> core/src/main/java/org/apache/accumulo/core/util/format/HexFormatter.java 1def7127a58c73313618c739bdd7f50bb8bc772f
> core/src/main/java/org/apache/accumulo/core/util/format/ShardedTableDistributionFormatter.java 577167a150b8861de714fdde79d2ba41ed06fb89
> core/src/main/java/org/apache/accumulo/core/util/format/StatisticsDisplayFormatter.java dd9de6ce6b967210322bec6c9b704f46b6031917
> core/src/test/java/org/apache/accumulo/core/util/format/DateStringFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/DefaultFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/DeleterFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/FormatterFactoryTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/HexFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/ShardedTableDistributionFormatterTest.java PRE-CREATION
> core/src/test/java/org/apache/accumulo/core/util/format/StatisticsDisplayFormatterTest.java PRE-CREATION
> core/src/test/resources/log4j.properties dfc93bf2f1bf04de85e5dd8fca1b966dc803e067
>
> Diff: https://reviews.apache.org/r/19428/diff/
>
>
> Testing
> -------
>
> New and old unit tests pass.
>
>
> Thanks,
>
> Mike Drob
>
>