ctubbsii commented on code in PR #5192:
URL: https://github.com/apache/accumulo/pull/5192#discussion_r1901325607
##########
server/base/src/test/java/org/apache/accumulo/server/init/InitializeTest.java:
##########
@@ -59,59 +62,60 @@ public void setUp() {
expect(sconf.get(Property.INSTANCE_SECRET))
.andReturn(Property.INSTANCE_SECRET.getDefaultValue()).anyTimes();
expect(sconf.get(Property.INSTANCE_ZK_HOST)).andReturn("zk1").anyTimes();
- zoo = createMock(ZooReaderWriter.class);
+ zk = createMock(ZooSession.class);
+ zrw = new ZooReaderWriter(zk);
}
@AfterEach
public void tearDown() {
- verify(sconf, zoo, fs);
+ verify(sconf, zk, fs);
}
@Test
public void testIsInitialized_HasInstanceId() throws Exception {
expect(fs.exists(anyObject(Path.class))).andReturn(true);
- replay(sconf, zoo, fs);
+ replay(sconf, zk, fs);
assertTrue(Initialize.isInitialized(fs, initConfig));
}
@Test
public void testIsInitialized_HasDataVersion() throws Exception {
expect(fs.exists(anyObject(Path.class))).andReturn(false);
expect(fs.exists(anyObject(Path.class))).andReturn(true);
- replay(sconf, zoo, fs);
+ replay(sconf, zk, fs);
assertTrue(Initialize.isInitialized(fs, initConfig));
}
@Test
public void testCheckInit_NoZK() throws Exception {
- expect(zoo.exists("/")).andReturn(false);
- replay(sconf, zoo, fs);
- assertThrows(IllegalStateException.class, () -> Initialize.checkInit(zoo,
fs, initConfig));
+ expect(zk.exists("/", null)).andReturn(null);
+ replay(sconf, zk, fs);
+ assertThrows(IllegalStateException.class, () -> Initialize.checkInit(zrw,
fs, initConfig));
}
@Test
public void testCheckInit_AlreadyInit() throws Exception {
- expect(zoo.exists("/")).andReturn(true);
+ expect(zk.exists("/", null)).andReturn(new Stat());
expect(fs.exists(anyObject(Path.class))).andReturn(true);
- replay(sconf, zoo, fs);
- assertThrows(IOException.class, () -> Initialize.checkInit(zoo, fs,
initConfig));
+ replay(sconf, zk, fs);
+ assertThrows(IOException.class, () -> Initialize.checkInit(zrw, fs,
initConfig));
}
@Test
public void testCheckInit_FSException() throws Exception {
- expect(zoo.exists("/")).andReturn(true);
+ expect(zk.exists("/", null)).andReturn(new Stat());
expect(fs.exists(anyObject(Path.class))).andThrow(new IOException());
- replay(sconf, zoo, fs);
- assertThrows(IOException.class, () -> Initialize.checkInit(zoo, fs,
initConfig));
+ replay(sconf, zk, fs);
+ assertThrows(IOException.class, () -> Initialize.checkInit(zrw, fs,
initConfig));
Review Comment:
No matter what we do, it's going to be a big change on its own. But I think
those ZR/ZRW changes can be done as a follow-on, since 1) I'm not sure exactly
which direction we should go (collapse their APIs into ZooSession vs. relocate
them and make their constructors protected, etc., and 2) this change is already
big enough.
I did notice that a lot of tests don't do things the same way... some create
a custom ZooReader around a mock ZooSession, some create a mock ZooReader, and
others use the MockServerContext and pass around ZooKeeper that way. I tried to
make some of these tests consistent, but that takes you down a very big rabbit
hole, and I came to the realization that the reason why all these are so
different is because our internal code passes around different things. If we
can simplify the internal code by reducing the number of objects passed around
(collapsing on a ServerContext or a ZooSession, and not passing around ZR/ZRW),
the test code also becomes simpler. So, I'd like to leave that as follow-on
work and not go down that rabbit hole (again) in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
