GiraffaFS / giraffa

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

Add FileSystemContractTestBase for Giraffa #127

Closed octo47 closed 9 years ago

octo47 commented 9 years ago

Hadoop has contract for filesystems, it implemented as testcase. We need this to be able to tests how our filesystem behaves.

octo47 commented 9 years ago

PR #128 adds tests, but several tests don't work. I override them to make test passable. We can address these broken tests in separate issues:

testMkdirsWithUmask() testOverwrite() testOverWriteAndRead() testDeleteRecursively() testRenameFileAsExistingDirectory() testRenameDirectoryAsExistingDirectory()

shvachko commented 9 years ago

Good idea to add FileSystemContractTest. Looking at the patch:

  1. Why does it need suit() method and reflections? It is a junit 3 test base, so it will run anything starting with "test". You should be able to just extend FileSystemContractTestBase and override tests that are not passing. And define setup(), teardown().
  2. LOG member is unused.
  3. Few long lines (> 80).
octo47 commented 9 years ago

Thank you for reviewing. Just extending is what I did initially. But we need before/after class initialization, which is missing in Junit3. So my attempt to add TestSetup or construct suite (and init all things before and shutdown after) evolutes to that mess of code. As for long lines, we are using 80 width?

octo47 commented 9 years ago

Fixed 2. and 3. Not sure what to do with 1.

shvachko commented 9 years ago

Yes, we use 80 width. Might want to configure your IDE. We also use tab width=2 as spaces.

I thought that setUp() is easy - if cluster is already up (cluster != null) then don't start it. tearDown() is harder as you need to know when the last test case is executed. Cannot think of anything but a static counter of test cases.

Or may be TestSuite is yet the right way. Did you try

public static Test suite() {
    return new TestSetup(new TestSuite(TestGiraffaFSContract.class)) {
        protected void setUp() throws Exception {
            // Start giraffa cluster
        }
        protected void tearDown() throws Exception {
            // Stop giraffa cluster
        }
    };
}

According to documentation of TestSuite it should add all the methods starting with "test" as test cases to the suite - no need for reflections.

octo47 commented 9 years ago

Ah, thats it. I tried to pass constructed object, but need class to be passed. Thanks. Now it works.

shvachko commented 9 years ago

There is 6 left-over unused imports now. Otherwise looks good. +1 to commit after removing those.

shvachko commented 9 years ago

Removed unused imports and committed. Thank you Andrey. Also added Issue #105 to CHANGES.txt as part of this commit.

octo47 commented 9 years ago

thank you.