Bigtop
  1. Bigtop
  2. BIGTOP-1316

enhance Shell for better checking of return code & output logging

    Details

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

      Description

      for the slider tests, we extended Shell with [SliderShellhttps://svn.apache.org/viewvc/incubator/slider/trunk/slider-funtest/src/main/groovy/org/apache/slider/funtest/framework/SliderShell.groovy?view=markup], adding some more features. Some of those are biased towards executing our entry point, but there are some useful core features worth pulling up to the base class

      1. toString() and dump() methods with diagnostics
      2. return code sign correction and assertion checking
      3. construction of bash environment and command lines

      The bash setup could be done with a class Bash extends Shell which is designed purely for bash execution

      1. BIGTOP-1316-001.patch
        2 kB
        Steve Loughran
      2. BIGTOP-1316-002.patch
        5 kB
        Steve Loughran
      3. bigtop-1316-003.patch
        5 kB
        Steve Loughran

        Issue Links

          Activity

          Hide
          Roman Shaposhnik added a comment -

          Steve Loughran would appreciate this extra functionality submitted as a patch!

          Show
          Roman Shaposhnik added a comment - Steve Loughran would appreciate this extra functionality submitted as a patch!
          Hide
          Steve Loughran added a comment -

          initial patch. Adds the ability to validate the sign corrected exit code against an expected one, with helper functions to exec a command and then perform this validation.

          the dump() method dumps the command and the output; it's called if the exit code is wrong

          Show
          Steve Loughran added a comment - initial patch. Adds the ability to validate the sign corrected exit code against an expected one, with helper functions to exec a command and then perform this validation. the dump() method dumps the command and the output; it's called if the exit code is wrong
          Hide
          jay vyas added a comment -

          Thanks Steve Loughran .

          Two quesitons: Partially due to my ignorance ;

          • But what about exit -2, and so on ? I think 255 is idiomatic for when another program uses an "out-of-range" exit value, so actually exit -1 should normally be mapped to 255, not -1.
          • I really like the idea of dumping logs on failure... even when trace isnt enabled. That will make tests results easier to debug. So – shall we merge the logic in the existing Shell constructor
             
             97     if (LOG.isTraceEnabled()) {
             98         if (ret != 0) {
             99            LOG.trace("return: $ret");
            100         }
            101         if (out.size() != 0) {
            102            LOG.trace("\n<stdout>\n${out.join('\n')}\n</stdout>");
            103         }
            104         if (err.size() != 0) {
            105            LOG.trace("\n<stderr>\n${err.join('\n')}\n</stderr>");
            

          with yours ?

          Show
          jay vyas added a comment - Thanks Steve Loughran . Two quesitons: Partially due to my ignorance ; But what about exit -2, and so on ? I think 255 is idiomatic for when another program uses an "out-of-range" exit value, so actually exit -1 should normally be mapped to 255, not -1. I really like the idea of dumping logs on failure... even when trace isnt enabled. That will make tests results easier to debug. So – shall we merge the logic in the existing Shell constructor 97 if (LOG.isTraceEnabled()) { 98 if (ret != 0) { 99 LOG.trace("return: $ret"); 100 } 101 if (out.size() != 0) { 102 LOG.trace("\n<stdout>\n${out.join('\n')}\n</stdout>"); 103 } 104 if (err.size() != 0) { 105 LOG.trace("\n<stderr>\n${err.join('\n')}\n</stderr>"); with yours ?
          Hide
          Steve Loughran added a comment -

          if someone does exit(-2), then the return code from the operation is -2. The purpose of the sign correction to ensure that the exit code raised is what you see in your assertis without having to know in your head that it magically becomes, in the case of exit -2, 254. This lets you make assertions that the result of an operation matches the exit code using the same constant. I still keep the uncorrected return code around for those people who like to keep their maintenance costs high.

          w.r.t debugging, Having things at trace level lets you see the output even for commands that do not fail this can be useful ; on a failure my asserts simply repeat this with a different log level. Which we can't easily do by sharing code unless I play games with closures and then pass in Log.trace() or LOG.error depending on the chosen log level.

          Show
          Steve Loughran added a comment - if someone does exit(-2) , then the return code from the operation is -2 . The purpose of the sign correction to ensure that the exit code raised is what you see in your assertis without having to know in your head that it magically becomes, in the case of exit -2, 254. This lets you make assertions that the result of an operation matches the exit code using the same constant . I still keep the uncorrected return code around for those people who like to keep their maintenance costs high. w.r.t debugging, Having things at trace level lets you see the output even for commands that do not fail this can be useful ; on a failure my asserts simply repeat this with a different log level. Which we can't easily do by sharing code unless I play games with closures and then pass in Log.trace() or LOG.error depending on the chosen log level.
          Hide
          jay vyas added a comment -

          Okay, so i guess you are saying

          • its a good thing to support negative error codes and not wrap them to 255 (i agree makes sense).
          • not much value in sharing the code for LOG.trace/info.

          cool. ill go ahead and test this out now - fyi going to use this as an opportunity to test out a local automated patch evaluator im working on for BIGTOP-1249.

          Show
          jay vyas added a comment - Okay, so i guess you are saying its a good thing to support negative error codes and not wrap them to 255 (i agree makes sense). not much value in sharing the code for LOG.trace/info. cool. ill go ahead and test this out now - fyi going to use this as an opportunity to test out a local automated patch evaluator im working on for BIGTOP-1249 .
          Hide
          jay vyas added a comment - - edited

          okay , well if I do patch -p1 < .... this applies cleanly. But in bigtop we usually are able to apply patches doing "git am".

          Do we support any kind of patch (p0 p1 git ....) Konstantin Boudnik ? Im asking because im currently writing the BIGTOP-1249 automated validator, and it failed to apply the patch (because it tried to do git am instead of patch -p0 <).

          Currently in our wiki, we suggest people create patches using git git format-patch HEAD^..HEAD --stdout > BIGTOP-1031.patch.

          .... regarding the *contents* of the patch : they seem good as per your responses above.

          Show
          jay vyas added a comment - - edited okay , well if I do patch -p1 < .... this applies cleanly. But in bigtop we usually are able to apply patches doing "git am". Do we support any kind of patch (p0 p1 git ....) Konstantin Boudnik ? Im asking because im currently writing the BIGTOP-1249 automated validator, and it failed to apply the patch (because it tried to do git am instead of patch -p0 < ). Currently in our wiki, we suggest people create patches using git git format-patch HEAD^..HEAD --stdout > BIGTOP-1031 .patch . .... regarding the * contents * of the patch : they seem good as per your responses above.
          Hide
          Konstantin Boudnik added a comment - - edited

          I think there won't be any hard to look inside of the patch and see if we need to use git apply or git -p0 apply ? What do you think? This way, we won't be imposing a particular expectations to form the patch.

          Show
          Konstantin Boudnik added a comment - - edited I think there won't be any hard to look inside of the patch and see if we need to use git apply or git -p0 apply ? What do you think? This way, we won't be imposing a particular expectations to form the patch.
          Hide
          jay vyas added a comment - - edited

          +1 Steve Loughran are you going to go ahead with commiting this ?

          sorry , i got so distracted with the tangential issues, I forgot to actually test the patch Just compiled itest after applying your patch, everything looks ok, save a few linux specific test failures error which happen when building on a mac.

          ((( And thanks BTW, its awesome that Slider is pushing back to bigtop )))

          Show
          jay vyas added a comment - - edited +1 Steve Loughran are you going to go ahead with commiting this ? sorry , i got so distracted with the tangential issues, I forgot to actually test the patch Just compiled itest after applying your patch, everything looks ok, save a few linux specific test failures error which happen when building on a mac. ((( And thanks BTW, its awesome that Slider is pushing back to bigtop )))
          Hide
          Steve Loughran added a comment -

          ...I see, I created a patch using git diff, and you want it the other way, formatted. I will know better next time. git apply would have worked.

          I think I have the rights to commit this...

          Show
          Steve Loughran added a comment - ...I see, I created a patch using git diff , and you want it the other way, formatted. I will know better next time. git apply would have worked. I think I have the rights to commit this...
          Hide
          Konstantin Boudnik added a comment -

          Is this logic

          +  void execute(int expectedExitCode, Object... args) {
          

          looks weird only to me? I think a much more clear semantic would be to return a code from the method to be checked up later on. I think this why they are called return code everywhere. What's the use case for this contract? Why can't it be done in a normal func(arg) -> value

          Show
          Konstantin Boudnik added a comment - Is this logic + void execute( int expectedExitCode, Object ... args) { looks weird only to me? I think a much more clear semantic would be to return a code from the method to be checked up later on. I think this why they are called return code everywhere. What's the use case for this contract? Why can't it be done in a normal func(arg) -> value
          Hide
          jay vyas added a comment -

          I think Steve's exec function is meant for implementing an assertExitCode functionality for syntactic sugar when testing exit codes .

          Show
          jay vyas added a comment - I think Steve's exec function is meant for implementing an assertExitCode functionality for syntactic sugar when testing exit codes .
          Hide
          Steve Loughran added a comment -

          the existing exec(args) does the exec and returns a Shell instance containing the result; the execute(exitCode, args) is, as jay points out, designed to make writing shell ops with expected exit codes trivial.

          Now, looking at my uses of it, having an expected exit code of 0 is the most common.

          Should we also shortcut execute(0, args with some operation like succeeds(args) ? or execToSuccess(args?

          Show
          Steve Loughran added a comment - the existing exec(args) does the exec and returns a Shell instance containing the result; the execute(exitCode, args) is, as jay points out, designed to make writing shell ops with expected exit codes trivial. Now, looking at my uses of it, having an expected exit code of 0 is the most common. Should we also shortcut execute(0, args with some operation like succeeds(args) ? or execToSuccess(args ?
          Hide
          Konstantin Boudnik added a comment -

          the existing exec(args) does the exec and returns a Shell instance containing the result

          that object also has return value in it, which can be validated upon (as you do).
          Perhaps, my main contention isn't against such functionality, but rather towards having a better name for the methods. execute and exec are too close to each other yet have a very different contracts. That's confusing.

          Another issue: I do not consider iTest code to be a test per se, hence please don't introduce the direct dependency to junit.

          Show
          Konstantin Boudnik added a comment - the existing exec(args) does the exec and returns a Shell instance containing the result that object also has return value in it, which can be validated upon (as you do). Perhaps, my main contention isn't against such functionality, but rather towards having a better name for the methods. execute and exec are too close to each other yet have a very different contracts. That's confusing. Another issue: I do not consider iTest code to be a test per se, hence please don't introduce the direct dependency to junit.
          Hide
          Steve Loughran added a comment -
          1. what name do you recommend?
          2. which exception should I throw? A RuntimeException or something else? If I could create a new one with the exit code and other details in, that would be ideal -it's what my original code did but I pulled that for dependency reasons
          Show
          Steve Loughran added a comment - what name do you recommend? which exception should I throw? A RuntimeException or something else? If I could create a new one with the exit code and other details in, that would be ideal -it's what my original code did but I pulled that for dependency reasons
          Hide
          jay vyas added a comment - - edited

          3. Konstantin Boudnik there are lots of junit deps in the itest packages (actually a whole package called junit )... so to be clear - why is it bad to have junit deps in Shell.groovy? I think i know the answer but just want to see if we're all on the same page.

          Show
          jay vyas added a comment - - edited 3. Konstantin Boudnik there are lots of junit deps in the itest packages (actually a whole package called junit )... so to be clear - why is it bad to have junit deps in Shell.groovy ? I think i know the answer but just want to see if we're all on the same page.
          Hide
          jay vyas added a comment - - edited

          MAybe a simpler way forward if we all agree is , Konstantin Boudnik and Steve Loughran : how about a class called bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/shell/JunitShell.groovy that extends Shell.groovy , with the functionality steve wants to use in slider?

          Show
          jay vyas added a comment - - edited MAybe a simpler way forward if we all agree is , Konstantin Boudnik and Steve Loughran : how about a class called bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/shell/JunitShell.groovy that extends Shell.groovy , with the functionality steve wants to use in slider?
          Hide
          Roman Shaposhnik added a comment -

          jay vyas I like the idea of keeping test-centric contracts separate from more generic functionality. +1 to your suggestion.

          Show
          Roman Shaposhnik added a comment - jay vyas I like the idea of keeping test-centric contracts separate from more generic functionality. +1 to your suggestion.
          Hide
          Konstantin Boudnik added a comment -

          what name do you recommend?

          Steve Loughran, I think with the jay vyas's extension idea we can move it into a separate class, perhaps TestShell or something like this. And then the name of the methods might not be that much of a
          contention anymore. What do you think?

          Show
          Konstantin Boudnik added a comment - what name do you recommend? Steve Loughran , I think with the jay vyas 's extension idea we can move it into a separate class, perhaps TestShell or something like this. And then the name of the methods might not be that much of a contention anymore. What do you think?
          Hide
          Steve Loughran added a comment -

          worksforme

          Show
          Steve Loughran added a comment - worksforme
          Hide
          Steve Loughran added a comment -

          ...though actually the output capturing/toString/exit code setting should still be in Shell. Its only the execution with an exit code check that is JUnit specific

          Show
          Steve Loughran added a comment - ...though actually the output capturing/toString/exit code setting should still be in Shell . Its only the execution with an exit code check that is JUnit specific
          Hide
          Konstantin Boudnik added a comment -

          I think you're right. Along with the code conversion utility method.

          Show
          Konstantin Boudnik added a comment - I think you're right. Along with the code conversion utility method.
          Hide
          Steve Loughran added a comment -

          updated patch, now created with git format-patch

          Show
          Steve Loughran added a comment - updated patch, now created with git format-patch
          Hide
          jay vyas added a comment -

          Looks good, i also ran the build , it all compiles and doesnt revert anything .

          Also now the changes to Shell.groovy are minimal it seems, so it should make the others happy .

          Any other thoughts ?

          Show
          jay vyas added a comment - Looks good, i also ran the build , it all compiles and doesnt revert anything . Also now the changes to Shell.groovy are minimal it seems, so it should make the others happy . Any other thoughts ?
          Hide
          Konstantin Boudnik added a comment - - edited

          Is it an intenation visibility layout:

          • void expectExitCode(int expectedExitCode, Object... args)
          • void expectSuccess(Object... args)
            yet
          • public void assertExitCode(int expectedExitCode)

          Do you real need an empty constructor

          +  JUnitShell() {
          +  }
          
          Show
          Konstantin Boudnik added a comment - - edited Is it an intenation visibility layout: void expectExitCode(int expectedExitCode, Object... args) void expectSuccess(Object... args) yet public void assertExitCode(int expectedExitCode) Do you real need an empty constructor + JUnitShell() { + }
          Hide
          jay vyas added a comment - - edited

          i guess it would be nice to (explicitly) say "public" on those 3 lines and remove the empty constructor.

          any other cleanup needed ?

          Show
          jay vyas added a comment - - edited i guess it would be nice to (explicitly) say "public" on those 3 lines and remove the empty constructor. any other cleanup needed ?
          Hide
          Steve Loughran added a comment -

          iteration 3

          1. consistent removal of "public" on all methods in junitshell, and in Shell.toString
          2. added javadocs to explain constructors, including what the empty constructor does
          Show
          Steve Loughran added a comment - iteration 3 consistent removal of "public" on all methods in junitshell, and in Shell.toString added javadocs to explain constructors, including what the empty constructor does
          Hide
          Konstantin Boudnik added a comment -

          +1

          Show
          Konstantin Boudnik added a comment - +1
          Hide
          jay vyas added a comment -

          Steve you can/ will commit this , i assume. if not just let us know we can commit for you .

          Show
          jay vyas added a comment - Steve you can/ will commit this , i assume. if not just let us know we can commit for you .
          Hide
          Steve Loughran added a comment -

          I can't seem to push this up...looks like I don't have the permissions. Can you apply it for now, and if I do have more changes to get in then I'll worry about access

          Show
          Steve Loughran added a comment - I can't seem to push this up...looks like I don't have the permissions. Can you apply it for now, and if I do have more changes to get in then I'll worry about access
          Hide
          Roman Shaposhnik added a comment -

          +1 and pushed!

          Show
          Roman Shaposhnik added a comment - +1 and pushed!
          Hide
          jay vyas added a comment -

          Its great to see other projects feeding contributions back into bigtop. Hope to see more of that in the future !

          Thanks Steve Loughran !!!!!

          Show
          jay vyas added a comment - Its great to see other projects feeding contributions back into bigtop. Hope to see more of that in the future ! Thanks Steve Loughran !!!!!

            People

            • Assignee:
              Steve Loughran
              Reporter:
              Steve Loughran
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development