> On May 23, 2016, 2:59 p.m., 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.
> 
> Daniel Gergely wrote:
>     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.

Hi Daniel,  

You're certainly correct about the ideal situation with respect to unit testing 
in this class.  Unfortunately, the current structure of the code (static 
registration of updaters) would make it difficult to unit test at all in the 
more direct way that you are describing.  

That being said, I'd always be more inclined to have more unit test coverage 
than less coverage, so for now I think it makes sense to have these tests in 
place, since they have been very useful in the past for preventing regressions.

In a future Ambari release, it might be a good idea to refactor the 
BlueprintConfigurationProcessor, to move some of the responsibilities to other 
dependencies that can be independently unit-tested.  It may also be a good idea 
to change the way that the property updaters are registered, so that these are 
non-static registrations.  Then, the approach you mentioned would work fine 
(unit tests to verify the updater implementations, unit tests to verify that 
specific properties are registered as expected).  

We would definitely need tests for the individual properties in some way, since 
many bugs involving component-specific handling have been resolved, and are 
verified by these existing tests.

Thanks.


- Robert


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


On May 24, 2016, 8:55 a.m., Daniel Gergely wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47726/
> -----------------------------------------------------------
> 
> (Updated May 24, 2016, 8:55 a.m.)
> 
> 
> 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 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  073e827 
> 
> Diff: https://reviews.apache.org/r/47726/diff/
> 
> 
> Testing
> -------
> 
> In progress...
> 
> 
> Thanks,
> 
> Daniel Gergely
> 
>

Reply via email to