GiraffaFS / giraffa

Giraffa FileSystem (Slack: giraffa-fs.slack.com)
https://giraffa.ci.cloudbees.com
Apache License 2.0
17 stars 6 forks source link

Increase test case coverage for TestGiraffaFSContract. #144

Closed shvachko closed 9 years ago

shvachko commented 9 years ago

A few tests in TestGiraffaFSContract are overridden as no-op to avoid the test failure. Some of them can in fact be fixed by adjusting the responses to RPC calls in Giraffa. Eventually TestGiraffaFSContract should pass for Giraffa entirely.

octo47 commented 9 years ago

took it to myself. progress can be tracked in branch issue-144. but it seems that this issue can generate a couple of smaller issues.

shvachko commented 9 years ago

Andrey, could post some description of what is done in the branch, like these are the failing tests, and this is how you propose to fix them. Splitting into smaller issues generally makes sense when the changes grows too big.

octo47 commented 9 years ago

I thought that it would be bigger. But now it happens that most tests can be fixed with just a couple of lines. Every commit in this branch has messsage on what was done. At the end I squash them and produce single commit message.

shvachko commented 9 years ago

Posting your commit messages. They will be lost after merging and removing the branch

octo47 commented 9 years ago

git merge --squash will create commit message with all commit messages of squashed commits. so it can be done right before final merge. but agree, we can keep changelog there.

shvachko commented 9 years ago

Looks like the right approach. There are two tests failing on your branch:

  1. TestGiraffaFS, because PathIsNotEmptyDirectoryException is not handled.
  2. TestRename, because it gets incorrect full path with missing slashes:
java.io.IOException: Cannot calculate key for a relative path: grfa:/user/shv/test

This should be related to the implementation of getScheme()

  1. And you can remove unused import of IOException in TestGiraffaFSContract
octo47 commented 9 years ago

Thank you for finding that. Fixed tests and added comments to tests, which couldn't be fixed right now. That mostly about rename seamantics, which has a long story in HADOOP-6240 so we either should make our behaviour match hdfs behaviour, or just left this as is. And for sure changes in behaviour are out of scope of this issue.

shvachko commented 9 years ago

+1 for the patch. We should make Giraffa rename behave as FSContract requires to be fully compatible. @milandesai should look at it. Agreed in another issue.

shvachko commented 9 years ago

Just committed this. Thank you, Andrey.