Uploaded image for project: 'Bigtop'
  1. Bigtop
  2. BIGTOP-1321

TestCLI: check assumptions before the test run

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 0.8.0
    • Fix Version/s: 0.8.0
    • Component/s: tests
    • Labels:
      None

      Description

      The TestCLI test cases has some assumptions which are not checked in the test code. This is a proposal to add checks for each important assumption which would make some cases fail if not met. This would save us some debugging time when setting up new test environment and would make this test easier to reuse.

      So far, I have identified these:

      • the effective user must be hdfs (a root for HDFS)
      • the HDFS trash feature must be disabled (`fs.trash.interval` should be 0)
      • hdfs://tmp/testcli_TIMESTAMP directory has been really created

      I expect there are additional assumptions worth checking, any other ideas?

      1. BIGTOP-1321.1.patch
        4 kB
        Martin Bukatovic
      2. BIGTOP-1321.2.patch
        2 kB
        Martin Bukatovic
      3. BIGTOP-1321.3.patch
        2 kB
        Martin Bukatovic

        Issue Links

          Activity

          Hide
          mbukatov Martin Bukatovic added a comment -

          I will try to provide an example patch tomorrow to better show what I have in mind.

          Show
          mbukatov Martin Bukatovic added a comment - I will try to provide an example patch tomorrow to better show what I have in mind.
          Hide
          jayunit100 jay vyas added a comment -

          Great find martin. Im linking to the above issues as well - we originally knew that TestCLI was a little bias to HDFS, but werent quite sure where to start abstracting it. this is a good concrete jira we can tackle

          Show
          jayunit100 jay vyas added a comment - Great find martin. Im linking to the above issues as well - we originally knew that TestCLI was a little bias to HDFS, but werent quite sure where to start abstracting it. this is a good concrete jira we can tackle
          Hide
          mbukatov Martin Bukatovic added a comment -

          Initial patch attached. I'm not sure if raising excpetion like that follows the code style, but I think that when setup fails or severe misconfiguration is detected, the test needs to fail with clear description without executing the test cases.

          Show
          mbukatov Martin Bukatovic added a comment - Initial patch attached. I'm not sure if raising excpetion like that follows the code style, but I think that when setup fails or severe misconfiguration is detected, the test needs to fail with clear description without executing the test cases.
          Hide
          mbukatov Martin Bukatovic added a comment -

          Fixing typo in the patch and flagging for review.

          Show
          mbukatov Martin Bukatovic added a comment - Fixing typo in the patch and flagging for review.
          Hide
          mbukatov Martin Bukatovic added a comment -

          See BIGTOP-1321.1.patch

          Show
          mbukatov Martin Bukatovic added a comment - See BIGTOP-1321 .1.patch
          Hide
          cos Konstantin Boudnik added a comment -

          Thanks Martin. A couple of comments:

          • instead of introducing a whole new exception class I'd rather use assertEquals from the standard JUnit. Say this
            +    if (shHDFS.getRet() != 0) {
            +      String err_out = StringUtils.join(shHDFS.getErr().toArray(), "\\n");
            +      throw new TestCLISetupException("hadoop fs -mkdir of testcli dir failed: " + err_out);
            +    }
            

            would look like

                Assert.assertEquals("hadoop fs -mkdir of testcli dir failed: " + 
                    StringUtils.join(shHDFS.getErr().toArray(), "\\n"), 0, shHDFS.getRet());
            
          • for {{ if (!"0".equals(conf.get("fs.trash.interval"))) }} do you think you'd be able to use {{conf.getInt("fs.trash.interval", 1) }} in combination with an assert?
          Show
          cos Konstantin Boudnik added a comment - Thanks Martin. A couple of comments: instead of introducing a whole new exception class I'd rather use assertEquals from the standard JUnit. Say this + if (shHDFS.getRet() != 0) { + String err_out = StringUtils.join(shHDFS.getErr().toArray(), "\\n" ); + throw new TestCLISetupException( "hadoop fs -mkdir of testcli dir failed: " + err_out); + } would look like Assert.assertEquals( "hadoop fs -mkdir of testcli dir failed: " + StringUtils.join(shHDFS.getErr().toArray(), "\\n" ), 0, shHDFS.getRet()); for {{ if (!"0".equals(conf.get("fs.trash.interval"))) }} do you think you'd be able to use {{conf.getInt("fs.trash.interval", 1) }} in combination with an assert?
          Hide
          mbukatov Martin Bukatovic added a comment -

          Addressing Konstantin's comments in BIGTOP-1321.2.patch

          I'm using conf.getInt("fs.trash.interval", 0) instead of conf.getInt("fs.trash.interval", 1) because default value is 0 (trash disabled).

          Show
          mbukatov Martin Bukatovic added a comment - Addressing Konstantin's comments in BIGTOP-1321 .2.patch I'm using conf.getInt("fs.trash.interval", 0) instead of conf.getInt("fs.trash.interval", 1) because default value is 0 (trash disabled).
          Hide
          cos Konstantin Boudnik added a comment -

          Yup, that's what I meant but failed to express.
          One more comment: you need to make indentation in the code blocks like this

          +    Assert.assertEquals("Creation of testcli dir should succeed and return 0" +
          +      " (but it failed with the following error message: " +
          +      StringUtils.join(shHDFS.getErr().toArray(), "\\n") + ")",
          +      0, shHDFS.getRet());
          

          to be 4 spaces as this is a continuation of a statement. The new patch looks good otherwise!

          Show
          cos Konstantin Boudnik added a comment - Yup, that's what I meant but failed to express. One more comment: you need to make indentation in the code blocks like this + Assert.assertEquals( "Creation of testcli dir should succeed and return 0" + + " (but it failed with the following error message: " + + StringUtils.join(shHDFS.getErr().toArray(), "\\n" ) + ")" , + 0, shHDFS.getRet()); to be 4 spaces as this is a continuation of a statement. The new patch looks good otherwise!
          Hide
          mbukatov Martin Bukatovic added a comment -

          Thanks for the feedback, attaching patch with coding style fixed.

          Show
          mbukatov Martin Bukatovic added a comment - Thanks for the feedback, attaching patch with coding style fixed.
          Hide
          cos Konstantin Boudnik added a comment -

          +1

          Show
          cos Konstantin Boudnik added a comment - +1
          Hide
          cos Konstantin Boudnik added a comment -

          Committed to the master! Thanks Martin!

          Show
          cos Konstantin Boudnik added a comment - Committed to the master! Thanks Martin!

            People

            • Assignee:
              mbukatov Martin Bukatovic
              Reporter:
              mbukatov Martin Bukatovic
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development