> On Jan. 27, 2015, 8:04 p.m., Christopher Tubbs wrote: > > TESTING.md, lines 20-53 > > <https://reviews.apache.org/r/30236/diff/5/?file=836999#file836999line20> > > > > This whole section feels like a maven tutorial. It might be sufficient > > to mention that unit tests are executed by the maven-surefire-plugin, and > > integration tests are executed by the maven-failsafe-plugin, and a link to > > the "Introduction to the Maven Build Lifecycle" site, and each of those > > plugins for more information. > > > > Some of the other non-maven content, like the minimal requirements, and > > the run-length seem appropriate and relevant, but there's a lot of detail > > here about how standard maven plugins are working, and there are better > > resources for that information than our particular execution of them. > > kturner wrote: > oh no more comments about TESTING.md. All I did in this patch was rename > it from TESTING to TESTING.md :) However I am ok with reviewing and > improving it here, its just that all of the comments about it were completely > unexpected. > > I don't feel like the following long sentence is equivalent to a maven > tutorial. I can work the links in, but I see no harm in leaving it. > > Integration tests can be run by invoking `mvn integration-test` at > the top of the Apache Accumulo source tree; however, > like `mvn package` being recommended for unit tests, `mvn verify` is > often the recommended avenue to run the integration tests. > > Do you want to just remove the following? > > Take note that when invoking the `integration-test` lifecycle phase, > other functions will also be enabled which include > static analysis (findbugs) and software license checks (release > analysis tool -- RAT). > > Christopher Tubbs wrote: > I just think it's unusual to describe how to execute each individual > phase in the maven build lifecycle. That's what I mean by "maven tutorial". > The build instructions describe `mvn package`. Here, it should just describe > `mvn verify`. After all, the integration tests get executed during the > integration-test phase of the build lifecycle, but they don't get checked for > errors until the verify phase anyway. So, integration testing is not actually > completed unless you execute `mvn verify`. So, some of this is technically > correct, but misleading, and doesn't do justice the actual behavior of these > goals. > > > Do you want to just remove the following? > > That would help, but I also think it would be better to simply describe > `mvn verify` instead of the other goals.
Changes in patch #7 only mention `mvn verify` - kturner ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30236/#review69865 ----------------------------------------------------------- On Jan. 30, 2015, 11:20 p.m., kturner wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30236/ > ----------------------------------------------------------- > > (Updated Jan. 30, 2015, 11:20 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-1515 > https://issues.apache.org/jira/browse/ACCUMULO-1515 > > > Repository: accumulo > > > Description > ------- > > Reorganized information in README and converted to markdown. > > At this point I like the INSTALL.md document, but do not really like the > content of the README.md ATM. Putting this up for review to get suggestions. > > See how the markdown looks on GH : > https://github.com/keith-turner/accumulo/tree/ACCUMULO-1515 > > > Diffs > ----- > > BUILD.md PRE-CREATION > INSTALL.md PRE-CREATION > NOTICE af212c2 > README 4ebb078 > README.md PRE-CREATION > TESTING cf2afba > TESTING.md PRE-CREATION > assemble/src/main/assemblies/component.xml 3f18da3 > > Diff: https://reviews.apache.org/r/30236/diff/ > > > Testing > ------- > > > Thanks, > > kturner > >
