> On Dec. 13, 2016, 3:26 p.m., Robert Levas wrote: > > ambari-agent/src/test/java/org/apache/ambari/tools/zk/ZkMigratorTest.java, > > line 38 > > <https://reviews.apache.org/r/54698/diff/1/?file=1582731#file1582731line38> > > > > This test suite should use mocks (example, `org.easymock.EasyMock`) > > rather than create a server and perform operations on it. This is more of > > an intergration test than a unit test. > > Attila Magyar wrote: > I strongly disagree with this one. This code talks to a 3rd party > zookeeper API. It would be required to mock objects we don't own and can't > control. The test would mimic the exact same behaviour than the code, leading > to brittle and implementation specific tests without adding too much -if any- > value to the correctness of the code. I wanted to check with this test that > the 3rd party API works the way I expected. > Introducing an ambari specific abstraction on top of the zookeeper API > and mocking that would be ok, but the integration test still would be needed > i think and there is not too much code there besides which talks to zookeeper. > > Robert Levas wrote: > What you are describing is an intergration test. For a unit test, I > would mockup the Zookeeper (client) class and ensure that the new code is > making the expected calls on it. We do not need to test to make sure that the > 3rd party Zookeeper code is correct.
Yes, because the code integrates to a 3rd party api. This at the boundary of our domain. You're right that we don't need to test that Zookeeper code is correct, but we need to test the integration of our code and the 3rdparty code is correct (whether it uses the api correctly, and our assumptions about the 3rd party api was correct). Using mocks at the domain boundary (mocking jdbc, entityManager, java builtins, etc) tend to lead fragile tests and duplications. Mocking stable interfaces of our domain created by us is ok because they are higher level abstractions and we can control their implementations. But this code has no such part. - Attila ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54698/#review159008 ----------------------------------------------------------- On Dec. 15, 2016, 9:26 a.m., Attila Magyar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54698/ > ----------------------------------------------------------- > > (Updated Dec. 15, 2016, 9:26 a.m.) > > > Review request for Ambari, Attila Doroszlai, Dmitro Lisnichenko, Jaimin > Jetly, Laszlo Puskas, Oliver Szabo, Robert Levas, and Sebastian Toader. > > > Bugs: AMBARI-19187 > https://issues.apache.org/jira/browse/AMBARI-19187 > > > Repository: ambari > > > Description > ------- > > Hadoop components need to establish a secure connection with ZooKeeper when > Kerberos is enabled. This involves the setup of the correct authentication > (JAAS config file) and authorization (per-component Kerberos-backed ACLs on > the znodes) between the service and ZooKeeper. Most services are able to set > these ACLs based on their config when the user enable kerberos. > When we disable kerberos again, the sasl ACL should be removed otherwise the > services won't be able to access their znodes. > > This issue is about introducing a new command (DISABLE_SECURITY) that will be > sent by the ambari server to the services upon the dekerberiztion process. > When a service receives this command it will be able to do the zookeeper > secure to unsecure migration process (e.g. removing sasl ACLs). > > Notable changes: > - Added a java command line tool to the agent project that can setAcls > recursively on a znode > - Modified the dekerberization workflow: > - 1. UI stops all services but zookeeper > - 2. 2 new stages was introduced in the backend (send DISABLE_SECURITY > command to the services, start zookeeper) > > > Diffs > ----- > > ambari-agent/pom.xml a8ed7f1 > ambari-agent/src/main/java/org/apache/ambari/tools/zk/ZkAcl.java > PRE-CREATION > ambari-agent/src/main/java/org/apache/ambari/tools/zk/ZkConnection.java > PRE-CREATION > ambari-agent/src/main/java/org/apache/ambari/tools/zk/ZkMigrator.java > PRE-CREATION > ambari-agent/src/test/java/org/apache/ambari/tools/zk/ZkMigratorTest.java > PRE-CREATION > > ambari-common/src/main/python/resource_management/core/resources/zkmigrator.py > PRE-CREATION > > ambari-common/src/main/python/resource_management/libraries/script/script.py > 584775e > ambari-server/pom.xml 48ddb52 > > ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java > 3261a56 > > ambari-server/src/main/java/org/apache/ambari/server/metadata/ActionMetadata.java > 0064662 > > ambari-server/src/test/java/org/apache/ambari/server/controller/KerberosHelperTest.java > 90f8098 > ambari-web/app/controllers/main/admin/kerberos/disable_controller.js > cec4503 > > Diff: https://reviews.apache.org/r/54698/diff/ > > > Testing > ------- > > Added unittests for ZkMigrator, KerberosHelperImpl > > Manual testings: > - created cluster with ambari > - enabled kerberos > - disabled kerberos > - checked if the DISABLE_SECURITY command was sent to the services > > > Ambari agent: > ---------------------------------------------------------------------- > Ran 450 tests in 10.634s > > Ambari server: > ---------------------------------------------------------------------- > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 34:44.448s > [INFO] Finished at: Tue Dec 13 14:29:00 CET 2016 > [INFO] Final Memory: 160M/798M > > > Thanks, > > Attila Magyar > >
