Add INodeManager test

[octo47]

We need to add INodeManager test to be sure, that tree traversal works as expected (potentially with different RowKey implementations)

[shvachko]

Not quite sure what this task is intended to do. Do you plan to do it yourself?

[octo47]

Yeah, I thought about that, but not right now. Currently there is no test for this very important class.

[weilintsaiWand]

I investigated this issue for a while and had some discussions with Konstantin & Plamen. Both my observations and feedbacks I got are summarized below.

My question about the intention

I've spent some time on Issue 147 but still have no idea of what to do exactly. First of all, I ran current unit tests set and found the code coverage is 97.5% for INodeManager. Which means although there's no specific tests for INodeManger, the class is covered pretty well by other tests. Considering that the class is a critical class, those "other tests" would fail if we have bugs in INodeManger. Further more, some methods in INodeManageralready have specific tests. For instance, we have TestXAttr for XAttr related methods As a result, I guess the intention of this issue is to focus on getDirectories() and getListing(), am I right?

Even if our goal is to verify getDirectories() and getListing(). As I mentioned before, the code coverage is pretty good now. There's not many things we can do on these two methods. I mean, I could add some tests but I don't see any significant advantages on it.

The discussion also mentioned that we may need tests for potentially different RowKey implementation. However, even if we have another RowKey implementation, say, FullPathRowKeyV2. The logic in INodeManager is still the same. Since we already have high code coverage for INodeManager, the current tests case should cover new RowKey implementation as well.

Plamen's feedback

So what I believe Andrey was going for was to provide a sort of "contract" test. Similar to the FileSystemContract tests he introduced.

The basic idea is that we should have some form of test thats says "this is what we expect INodeManager to do".

Like "listDirectories" should: 1) Get Hbase table client. 2) Fetch some sort of iterator. 3) Convert results into FileStatuses.

Stuff of that nature.

It might fall into the category of "over engineering" at the moment.

I actually think its not a good idea to make such a test at the moment because INodeManager is HBase client dependent at the moment and we will make it extendable to support more than just HBase in the future.

Konstantin's feedback

Why don't you post your observations in the issue. You identified 97.5% coverage for INodeManger, which is probably more than other classes in giraffa. This is a result by itself. There are different ways to complete a task. An investigation that shows nothing need to be done is one of the ways. Especially given the vague description of intentions.

Reference Current Code Coverage of Key Classes