Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: multipart/alternative;
 boundary="===============7066769779296176457=="
MIME-Version: 1.0
Subject: Re: Review Request 19592: ACCUMULO-2470 - unit tests for server/base
From: "Bill Havanki" <bhavanki@clouderagovt.com>
To: "Bill Havanki" <bhavanki@clouderagovt.com>,
 "accumulo" <dev@accumulo.apache.org>, "Mike Drob" <mdrob@mdrob.com>
Date: Tue, 25 Mar 2014 16:55:19 -0000
Message-ID: <20140325165519.8507.14538@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Bill Havanki" <noreply@reviews.apache.org>
References: <20140325163300.8507.38043@reviews.apache.org>
In-Reply-To: <20140325163300.8507.38043@reviews.apache.org>
Reply-To: "Bill Havanki" <bhavanki@clouderagovt.com>

--===============7066769779296176457==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


> On March 25, 2014, 12:33 p.m., Mike Drob wrote:
> > server/base/src/test/java/org/apache/accumulo/server/problems/ProblemReportingIteratorTest.java, line 79
> > <https://reviews.apache.org/r/19592/diff/1/?file=534282#file534282line79>
> >
> >     What's the advantage of using a mock Range here?

For one thing, it saves me from having to figure out a good real Range object. Also, it ensures that the test makes no assumptions about what the Range is, so the test would apply for any Range.


> On March 25, 2014, 12:33 p.m., Mike Drob wrote:
> > server/base/src/test/java/org/apache/accumulo/server/tablets/TabletTimeTest.java, line 112
> > <https://reviews.apache.org/r/19592/diff/1/?file=534285#file534285line112>
> >
> >     Why is this test ignored?

Because of ACCUMULO-2523, which I filed after realizing this test would fail. So now, the assignee for that already has a test available. Little bit of TDD. :)


> On March 25, 2014, 12:33 p.m., Mike Drob wrote:
> > server/base/src/test/java/org/apache/accumulo/server/util/AdminCommandsTest.java, line 49
> > <https://reviews.apache.org/r/19592/diff/1/?file=534286#file534286line49>
> >
> >     Should we assert something?

I'd like to, but I'm not sure what. The other command tests (besides StopMasterCommand) just check that the annotated parameter definitions have expected default values, but this command has none. The class itself is empty.

This test is really here for coverage, but I'd be much happier with an assertion in there. If you have a suggestion, hit me with it!


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19592/#review38443
-----------------------------------------------------------


On March 24, 2014, 4:19 p.m., Bill Havanki wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19592/
> -----------------------------------------------------------
> 
> (Updated March 24, 2014, 4:19 p.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-2470
>     https://issues.apache.org/jira/browse/ACCUMULO-2470
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> A variety of low-hanging fruit unit tests to increase branch coverage in the server/base module to 15% and class coverage to 25%.
> 
> The most important part of this review is the set of changes to ProblemReport, done to enable testing of it. Mostly, existing methods were augmented with new ones that take in parameters like ZooReaderWriter and Instance objects, which can be set or mocked at test time.
> 
> 
> Diffs
> -----
> 
>   server/base/src/main/java/org/apache/accumulo/server/problems/ProblemReport.java fec4e550a743bbf36d92688c5ddf5da561d8f715 
>   server/base/src/test/java/org/apache/accumulo/server/AccumuloTest.java PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/ServerOptsTest.java PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/conf/TableConfigurationTest.java PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/master/state/MergeInfoTest.java PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/master/state/TabletLocationStateTest.java PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/problems/ProblemReportTest.java PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/problems/ProblemReportingIteratorTest.java PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/tablets/LogicalTimeTest.java PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/tablets/MillisTimeTest.java PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/tablets/TabletTimeTest.java PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/util/AdminCommandsTest.java PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/util/FileInfoTest.java PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/util/FileUtilTest.java PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/util/TServerUtilsTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19592/diff/
> 
> 
> Testing
> -------
> 
> Unit tests pass.
> 
> 
> Thanks,
> 
> Bill Havanki
> 
>


--===============7066769779296176457==--