> On máj. 23, 2016, 2:59 du, Robert Nettleton wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java, > > line 2395 > > <https://reviews.apache.org/r/47726/diff/1/?file=1391619#file1391619line2395> > > > > +1 for Sebastian's comment, a unit test should definitely be added here > > to verify this change. > > Daniel Gergely wrote: > We were talking about this unit test thing. These are static variables, > so I can test if all the properties are there in the map. However it is not > easy to test whether the correct PropertyUpdater is used with the correct > component. These are not visible from outside, so a complex mocking or using > Reflection could made it possible, but these are messy things. So he dropped > the issue here. > > Robert Nettleton wrote: > Hi Daniel, > > It should be pretty simple to unit test this issue, by verifiying that > this property is updated as expected, when the expected set of components are > included in the Blueprint/Cluster Configuration. There is no need to verify > the static maps being modified, but there is definitely a need to make sure > that this property is handled correctly by the Blueprint export process, so I > still think a unit test is necessary here. > > Thanks. > > Robert Nettleton wrote: > Here's an example of a unit test that verifies that the property updaters > are handling Blueprint exports correctly: > > > org.apache.ambari.server.controller.internal.BlueprintConfigurationProcessorTest#testYarnConfigExported > > There are many more examples of this in this test case as well. > > Thanks.
I added the test. However I have a different opinion on how to build up tests. Blueprint processor is way too complex, so it is hard to separate responsibilities. In an ideal world I would write tests for the property updaters, and not for all the properties that use these updaters. If property updaters are executed successfully, then all the properties that use them must be replaced correctly. Testing if all the properties have the correct updater with correct parameter is a separated responsibility. In the case we want to test whether replacement for each property is done correctly, that is rather a system test than a unit test, since we need a complete system to test on. No offense, this is just my way of thinking. - Daniel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47726/#review134358 ----------------------------------------------------------- On máj. 23, 2016, 2:07 du, Daniel Gergely wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47726/ > ----------------------------------------------------------- > > (Updated máj. 23, 2016, 2:07 du) > > > Review request for Ambari, Balázs Bence Sári, Laszlo Puskas, Oliver Szabo, > Robert Nettleton, Sandor Magyari, Sumit Mohanty, and Sebastian Toader. > > > Bugs: AMBARI-16820 > https://issues.apache.org/jira/browse/AMBARI-16820 > > > Repository: ambari > > > Description > ------- > > MultiHostUpdater was not assigned to the property > hive.llap.zk.sm.connectionString > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java > 9cabdbb > > Diff: https://reviews.apache.org/r/47726/diff/ > > > Testing > ------- > > In progress... > > > Thanks, > > Daniel Gergely > >
