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

Fix the shell-object bug in HttpFs smoke tests

    Details

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

      Description

      jun aoki has identified a bug in HttpFs smoke tests, where a different shell object was used to verify the return code from the command, than the one that really executed this command:

      110         sh.exec("curl -i -X DELETE '$HTTPFS_PREFIX$testHttpFsFolder?user.name=$USERNAME&op=DELETE'");
      111         assertTrue("curl command to delete a dir failed", shHDFS.getRet() == 0);
      
      1. BIGTOP-1246.1.patch
        5 kB
        Adam Kawa
      2. BIGTOP-1246.2.patch
        5 kB
        Adam Kawa

        Activity

        Hide
        kawaa Adam Kawa added a comment -

        Sure, thanks!

        Show
        kawaa Adam Kawa added a comment - Sure, thanks!
        Hide
        cos Konstantin Boudnik added a comment -

        Committed to the master. Thanks Adam!

        Please address the issues from my last comment

        Show
        cos Konstantin Boudnik added a comment - Committed to the master. Thanks Adam! Please address the issues from my last comment
        Hide
        cos Konstantin Boudnik added a comment -

        new patch looks good. +1 I will commit it now.

        I have also noticed a few issues in the test:

        • the package name is wrong. Either the path to test needs to be fixed or the package name changed. I prefer the latter.
        • there are unused imports
        • there's a statement like exists == true where exists is a boolean variable. You don't need '=='' here.

        Could you please address it in a separate JIRA?

        Show
        cos Konstantin Boudnik added a comment - new patch looks good. +1 I will commit it now. I have also noticed a few issues in the test: the package name is wrong. Either the path to test needs to be fixed or the package name changed. I prefer the latter. there are unused imports there's a statement like exists == true where exists is a boolean variable. You don't need '=='' here. Could you please address it in a separate JIRA?
        Hide
        kawaa Adam Kawa added a comment -

        OK. I see.

        To make it more clear, instead shHDFS, I introduced shUSERNAME = new Shell("/bin/bash", USERNAME) and I use it for a cleanup of directories (because these directories were created with user.name=USERNAME via httpfs).

        Show
        kawaa Adam Kawa added a comment - OK. I see. To make it more clear, instead shHDFS, I introduced shUSERNAME = new Shell("/bin/bash", USERNAME) and I use it for a cleanup of directories (because these directories were created with user.name=USERNAME via httpfs).
        Hide
        cos Konstantin Boudnik added a comment -

        I think the patch is going too far. I believe the intention of the test is to run some httpfs commands as a generic user via curl and then verify the codes (that's where the bug needs to be fixed). However, removing shHDFS seems like an overreach, as the cleanup needs to be done as the owner of the directories. These two users can be different hence the need for two different shell objects.

        Your patch only needs to address the asserts part in lines 101, 111, 134, 145, 148

        Show
        cos Konstantin Boudnik added a comment - I think the patch is going too far. I believe the intention of the test is to run some httpfs commands as a generic user via curl and then verify the codes (that's where the bug needs to be fixed). However, removing shHDFS seems like an overreach, as the cleanup needs to be done as the owner of the directories. These two users can be different hence the need for two different shell objects. Your patch only needs to address the asserts part in lines 101, 111, 134, 145, 148
        Hide
        plinnell Peter Linnell added a comment -

        Not sure how groovy handles this, but keep in mind we have standardized on /bin/bash to ensure consistent behavior throughout Bigtop.

        Show
        plinnell Peter Linnell added a comment - Not sure how groovy handles this, but keep in mind we have standardized on /bin/bash to ensure consistent behavior throughout Bigtop.
        Hide
        kawaa Adam Kawa added a comment -

        The patch removes a useless "shHDFS" object and uses "sh" everywhere.

        Show
        kawaa Adam Kawa added a comment - The patch removes a useless "shHDFS" object and uses "sh" everywhere.

          People

          • Assignee:
            kawaa Adam Kawa
            Reporter:
            kawaa Adam Kawa
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development