> On Sept. 2, 2014, 6:32 p.m., Josh Elser wrote: > > test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java, line 34 > > <https://reviews.apache.org/r/25260/diff/1/?file=674070#file674070line34> > > > > It'd be much nicer to have a test name that is more meaningful than a > > ticket. Having the ticket referenced in a comment is useful if I want to > > find out more, but at first glance it doesn't tell me anything about what > > it's testing. > > Mike Drob wrote: > Why is it even useful as a comment? It should already be in the commit > message and 'git blame' will tell you who and where it came from. > > Josh Elser wrote: > That's valid -- a bit more personal preference, I suppose. I prefer to > have information in the code rather than have to look at the history to find > when it was introduced, especially when the most recent change isn't from the > ticket that originally introduced it (when blame would be wrong).
One can't assume that a person inspecting the released code has access to the history or the JIRA. Those are nice supplemental resources, but understanding what is being tested, at a basic level, should not depend on them. A meaningful test name gives some scope to the issue in log output without a lookup, regardless of access to history or JIRA, so I think the name change suggestion is a good idea. Further, Josh's suggestion to bump the JIRA number to a comment I think also is a good idea, because it provides additional linkage to the JIRA in the absence of the git history. - Christopher ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25260/#review52094 ----------------------------------------------------------- On Sept. 2, 2014, 1:45 p.m., kturner wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25260/ > ----------------------------------------------------------- > > (Updated Sept. 2, 2014, 1:45 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-3096 > https://issues.apache.org/jira/browse/ACCUMULO-3096 > > > Repository: accumulo > > > Description > ------- > > Throw exception on metadata constraint violation, instead of retrying > > > Diffs > ----- > > core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7 > > server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java > 463ca57 > test/src/test/java/org/apache/accumulo/test/Accumulo3096IT.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/25260/diff/ > > > Testing > ------- > > Ran mvn package w/o incident . Currently running mvn verify. > > > Thanks, > > kturner > >
