----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52457/#review151171 -----------------------------------------------------------
Ship it! Overall, the patch looks fine to me, just a few comments below. We should be careful to make sure that the categorization of unit tests, which is a useful feature for many reasons, does not encourage developers to only run certain subsuites prior to checkin. Thanks. pom.xml (line 679) <https://reviews.apache.org/r/52457/#comment219397> I think the overall scheme to categorize tests sounds like a good idea. However, I do have some concerns about categorizing tests as "fast" or "slow". Do we have any specific guidelines for marking a test as either category? I believe there might be maven plugins to track the slowest tests in a given unit test run, so that might be a good place to start to determine which tests to mark as either category. This could potentially become a slippery slope, though, in which developers only run the fast tests prior to checkin. I would definitely be concerned about that. utility/src/main/java/category/AlertTest.java (line 1) <https://reviews.apache.org/r/52457/#comment219398> Having to create these empty interfaces to define new test categories seems problematic to me. That being said, based on how the Category annotation is defined, there might not be an option, so this might be the only way to use this annotation. - Robert Nettleton On Sept. 30, 2016, 11:10 p.m., Alejandro Fernandez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52457/ > ----------------------------------------------------------- > > (Updated Sept. 30, 2016, 11:10 p.m.) > > > Review request for Ambari, Andrew Onischuk, Dmytro Grinenko, Di Li, Dmitro > Lisnichenko, Jonathan Hurley, Nate Cole, Sid Wagle, and Tim Thorpe. > > > Bugs: AMBARI-14439 > https://issues.apache.org/jira/browse/AMBARI-14439 > > > Repository: ambari > > > Description > ------- > > Categorize the unit tests so we that "mvn test -P $PROFILE" command can run > only the desired category in order to run the tests faster. > > > Diffs > ----- > > ambari-logsearch/pom.xml 0445c39 > ambari-metrics/ambari-metrics-timelineservice/pom.xml 87e3423 > ambari-metrics/pom.xml 6ab8c60 > ambari-server/pom.xml e37accd > > ambari-server/src/test/java/org/apache/ambari/server/state/alerts/AggregateAlertListenerTest.java > 85dedba > > ambari-server/src/test/java/org/apache/ambari/server/state/alerts/AlertDefinitionEqualityTest.java > 3cc84c0 > > ambari-server/src/test/java/org/apache/ambari/server/state/alerts/AlertDefinitionHashTest.java > c534c4a > > ambari-server/src/test/java/org/apache/ambari/server/state/alerts/AlertEventPublisherTest.java > 76aa2e4 > > ambari-server/src/test/java/org/apache/ambari/server/state/alerts/AlertReceivedListenerTest.java > 7aef175 > > ambari-server/src/test/java/org/apache/ambari/server/state/alerts/AlertStateChangedEventTest.java > 7964d14 > > ambari-server/src/test/java/org/apache/ambari/server/state/alerts/InitialAlertEventTest.java > fc4803b > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosComponentDescriptorTest.java > 201d84e > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosConfigurationDescriptorTest.java > 4f2a2f5 > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosDescriptorTest.java > 0070e6d > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosDescriptorUpdateHelperTest.java > fca2f1f > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosIdentityDescriptorTest.java > ef1c7bb > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosKeytabDescriptorTest.java > 79350eb > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosPrincipalDescriptorTest.java > 635cc30 > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosServiceDescriptorTest.java > 9896317 > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/VariableReplacementHelperTest.java > 8be0eb9 > > ambari-server/src/test/java/org/apache/ambari/server/state/stack/ConfigUpgradePackTest.java > 388a81f > > ambari-server/src/test/java/org/apache/ambari/server/state/stack/ConfigUpgradeValidityTest.java > bf716b7 > > ambari-server/src/test/java/org/apache/ambari/server/state/stack/OSFamilyTest.java > 9654dc3 > > ambari-server/src/test/java/org/apache/ambari/server/state/stack/UpgradePackTest.java > 97e50c3 > pom.xml 7ca4ba4 > utility/pom.xml PRE-CREATION > utility/src/main/java/category/AlertTest.java PRE-CREATION > utility/src/main/java/category/AmbariUpgradeTest.java PRE-CREATION > utility/src/main/java/category/BlueprintTest.java PRE-CREATION > utility/src/main/java/category/FastTest.java PRE-CREATION > utility/src/main/java/category/KerberosTest.java PRE-CREATION > utility/src/main/java/category/MetricsTest.java PRE-CREATION > utility/src/main/java/category/SlowTest.java PRE-CREATION > utility/src/main/java/category/StackUpgradeTest.java PRE-CREATION > > Diff: https://reviews.apache.org/r/52457/diff/ > > > Testing > ------- > > Verified, > mvn clean test > mvn clean test -P FastTests > > > Thanks, > > Alejandro Fernandez > >
