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

FailureExecutor breaks smoke tests : Smoke tests should run from source

    Details

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

      Description

      Duh : By adding a hard requirement on itest-snapshot-0.9.0, we now break the existing mapreduce smoke tests.

      Maybe i can add ITest as a compiled, rather than jar file, dependency for smoke-tests . let me look into it

      1. BIGTOP-1524.patch
        5 kB
        jay vyas
      2. BIGTOP-1524.patch
        5 kB
        jay vyas
      3. BIGTOP-1524.patch
        5 kB
        jay vyas
      4. BIGTOP-1524.patch
        5 kB
        jay vyas
      5. BIGTOP-1524.patch
        5 kB
        jay vyas

        Issue Links

          Activity

          Hide
          jayunit100 jay vyas added a comment - - edited

          okay ! heres a patch to upgrade smoke-tests so that they work again. There were a bug in the Failure Executor that needed to be patched, and also, i needed to add source dependencies on itest.

          This is much better ! *it means that when we run gradle smoke-test recipes, we actually use iTest source - so it provides further automated validation of the bigtop codebase. It was also necessary since the introduction of the failureVars, since those arent published to Itest.

          • There was a minor bug in FailureExecutor which caused booleans to be parsed as "true" on accident .
          • This fixes that, and also updates the smoke-tests.sh script in vagrant recipe to run clean before testing.
          • Finally, it also adds iTest to the source path, so that smoke tests (for mapreduce) no longer require an ITest jar.

          Later we can upgrade all smokes (pig,mahout,hive...) to use the itest sources on maste rather than jarfiler, but for now, this is the minimal patch (mapreduce) to fix the problem.

          looks okay ?

          Show
          jayunit100 jay vyas added a comment - - edited okay ! heres a patch to upgrade smoke-tests so that they work again. There were a bug in the Failure Executor that needed to be patched, and also, i needed to add source dependencies on itest. This is much better ! *it means that when we run gradle smoke-test recipes, we actually use iTest source - so it provides further automated validation of the bigtop codebase. It was also necessary since the introduction of the failureVars , since those arent published to Itest. There was a minor bug in FailureExecutor which caused booleans to be parsed as "true" on accident . This fixes that, and also updates the smoke-tests.sh script in vagrant recipe to run clean before testing. Finally, it also adds iTest to the source path, so that smoke tests (for mapreduce) no longer require an ITest jar. Later we can upgrade all smokes (pig,mahout,hive...) to use the itest sources on maste rather than jarfiler, but for now, this is the minimal patch (mapreduce) to fix the problem. looks okay ?
          Hide
          jayunit100 jay vyas added a comment -

          looks like i have the wrong commit in this patch, let me re attach

          Show
          jayunit100 jay vyas added a comment - looks like i have the wrong commit in this patch, let me re attach
          Hide
          cos Konstantin Boudnik added a comment -
          • let's document the use of the sys. props System.getProperty("useFailureProperties"...) in the JavaDoc or readme or else, so the next user doesn't need to fish in the code
          • This looks like a formatting change that doesn't belong here
            -    switch (OS.linux_flavor) {
            +   switch (OS.linux_flavor) {
            
          • I am not sure it's a good idea to expose the innards of the test framework as a test
            +      "TestHadoopExamples.groovy",
            +      "FailureVars.groovy"
            
          • Is this intentional?
            -    //print("Exclude? ${filename} ... ")
            +    print("Exclude? ${filename} ... ")
            ....
            -    //println("Keep = ${keep_this_test} "+filename);
            +    println("Keep = ${keep_this_test} "+filename);
            +    
            
          • if you aren't using this line, why not to remove it?
            -  testCompile group: 'org.apache.bigtop.itest', name: 'itest-common', version: itestVersion, transitive: 'true'
            +  // testCompile group: 'org.apache.bigtop.itest', name: 'itest-common', version: itestVersion, transitive: 'true'
            
          • seems like a formatting change, irrelevant to the fix
            -      srcDirs = [
            -          'conf/',
            -      ]
            -    }
            +       srcDirs = ['conf/']
            +     }
            +    
            
          • the only line that is really relevant to this JIRA is this
            +import org.apache.bigtop.itest.failures.FailureVars
            

            and it's an interesting one because without this import TestHadoopExamples compiles just fine. Perhaps it is specific to the smoke tests organization?

          Show
          cos Konstantin Boudnik added a comment - let's document the use of the sys. props System.getProperty("useFailureProperties"...) in the JavaDoc or readme or else, so the next user doesn't need to fish in the code This looks like a formatting change that doesn't belong here - switch (OS.linux_flavor) { + switch (OS.linux_flavor) { I am not sure it's a good idea to expose the innards of the test framework as a test + "TestHadoopExamples.groovy" , + "FailureVars.groovy" Is this intentional? - //print( "Exclude? ${filename} ... " ) + print( "Exclude? ${filename} ... " ) .... - //println( "Keep = ${keep_this_test} " +filename); + println( "Keep = ${keep_this_test} " +filename); + if you aren't using this line, why not to remove it? - testCompile group: 'org.apache.bigtop.itest', name: 'itest-common', version: itestVersion, transitive: ' true ' + // testCompile group: 'org.apache.bigtop.itest', name: 'itest-common', version: itestVersion, transitive: ' true ' seems like a formatting change, irrelevant to the fix - srcDirs = [ - 'conf/', - ] - } + srcDirs = ['conf/'] + } + the only line that is really relevant to this JIRA is this + import org.apache.bigtop.itest.failures.FailureVars and it's an interesting one because without this import TestHadoopExamples compiles just fine. Perhaps it is specific to the smoke tests organization?
          Hide
          jayunit100 jay vyas added a comment -

          thanks cos, lemme clean this up and resubmit. ill also test the failurevars one. ill attach it now for historical purposes

          Show
          jayunit100 jay vyas added a comment - thanks cos, lemme clean this up and resubmit. ill also test the failurevars one. ill attach it now for historical purposes
          Hide
          jayunit100 jay vyas added a comment - - edited
          • fixed comment (removed it)
          • clean up changes to srcDir
          • kept the FailureVars* import - seems to be required. (In any case, its the right thing to explicit import)

          tested in VMs, compile phase now passes w no errors and mapred tests work.

          Executing task ':mapreduce:compileTestGroovy' (up-to-date check took 0.939 secs) due to:
            Output file /bigtop-home/bigtop-tests/smoke-tests/mapreduce/build/classes/test has changed.
            Output file /bigtop-home/bigtop-tests/smoke-tests/mapreduce/build/classes/test/org/apache/bigtop/itest/hadoop/mapreduce/TestHadoopExamples.class has been removed.
            Output file /bigtop-home/bigtop-tests/smoke-tests/mapreduce/build/classes/test/org/apache/bigtop/itest/hadoop/mapreduce/TestHadoopExamples$_generateTests_closure1.class has been removed.
          Executing org.gradle.api.internal.tasks.compile.ApiGroovyCompiler@5efaa81f in compiler daemon.
          Successfully executed org.gradle.api.internal.tasks.compile.ApiGroovyCompiler@5efaa81f in compiler daemon.
          :mapreduce:compileTestGroovy (Thread[main,5,main]) completed. Took 1.606 secs.
          :mapreduce:processTestResources (Thread[main,5,main]) started.
          :mapreduce:processTestResources
          Skipping task ':mapreduce:processTestResources' as it has no source files.
          :mapreduce:processTestResources UP-TO-DATE
          :mapreduce:processTestResources (Thread[main,5,main]) completed. Took 0.003 secs.
          :mapreduce:testClasses (Thread[main,5,main]) started.
          :mapreduce:testClasses
          Skipping task ':mapreduce:testClasses' as it has no actions.
          :mapreduce:testClasses (Thread[main,5,main]) completed. Took 0.001 secs.
          :mapreduce:test (Thread[main,5,main]) started.
          :mapreduce:test
          Executing task ':mapreduce:test' (up-to-date check took 0.453 secs) due to:
            No history is available.
          ENV VARIABLE: HADOOP_CONF_DIR = /etc/hadoop/conf/
          Starting process 'Gradle Test Executor 2'. Working directory: /bigtop-home/bigtop-tests/smoke-tests/mapreduce Command: /usr/lib/jvm/java-1.7.0-openjdk-1.7.0.71.x86_64/bin/java -Djava.security.manager=jarjar.org.gradle.process.internal.child.BootstrapSecurityManager -Dfile.encoding=UTF-8 -Duser.country=US -Duser.language=en -Duser.variant -ea -cp /root/.gradle/caches/2.1/workerMain/gradle-worker.jar jarjar.org.gradle.process.internal.launcher.GradleWorkerMain 'Gradle Test Executor 2'
          Successfully started process 'Gradle Test Executor 2'
          Gradle Test Executor 2 started executing tests.
          
          org.apache.bigtop.itest.hadoop.mapreduce.TestHadoopExamples STANDARD_ERROR
              log4j:WARN No appenders could be found for logger (java.lang.Object).
              log4j:WARN Please initialize the log4j system properly.
              log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info.
          > Building 73% > :mapreduce:tes
          

          let me know if this is okay to commit, would like to get it in asap so that the vagrant recipes in head are fixed. thanks!

          Show
          jayunit100 jay vyas added a comment - - edited fixed comment (removed it) clean up changes to srcDir kept the FailureVars* import - seems to be required. (In any case, its the right thing to explicit import) tested in VMs, compile phase now passes w no errors and mapred tests work. Executing task ':mapreduce:compileTestGroovy' (up-to-date check took 0.939 secs) due to: Output file /bigtop-home/bigtop-tests/smoke-tests/mapreduce/build/classes/test has changed. Output file /bigtop-home/bigtop-tests/smoke-tests/mapreduce/build/classes/test/org/apache/bigtop/itest/hadoop/mapreduce/TestHadoopExamples.class has been removed. Output file /bigtop-home/bigtop-tests/smoke-tests/mapreduce/build/classes/test/org/apache/bigtop/itest/hadoop/mapreduce/TestHadoopExamples$_generateTests_closure1.class has been removed. Executing org.gradle.api.internal.tasks.compile.ApiGroovyCompiler@5efaa81f in compiler daemon. Successfully executed org.gradle.api.internal.tasks.compile.ApiGroovyCompiler@5efaa81f in compiler daemon. :mapreduce:compileTestGroovy (Thread[main,5,main]) completed. Took 1.606 secs. :mapreduce:processTestResources (Thread[main,5,main]) started. :mapreduce:processTestResources Skipping task ':mapreduce:processTestResources' as it has no source files. :mapreduce:processTestResources UP-TO-DATE :mapreduce:processTestResources (Thread[main,5,main]) completed. Took 0.003 secs. :mapreduce:testClasses (Thread[main,5,main]) started. :mapreduce:testClasses Skipping task ':mapreduce:testClasses' as it has no actions. :mapreduce:testClasses (Thread[main,5,main]) completed. Took 0.001 secs. :mapreduce:test (Thread[main,5,main]) started. :mapreduce:test Executing task ':mapreduce:test' (up-to-date check took 0.453 secs) due to: No history is available. ENV VARIABLE: HADOOP_CONF_DIR = /etc/hadoop/conf/ Starting process 'Gradle Test Executor 2'. Working directory: /bigtop-home/bigtop-tests/smoke-tests/mapreduce Command: /usr/lib/jvm/java-1.7.0-openjdk-1.7.0.71.x86_64/bin/java -Djava.security.manager=jarjar.org.gradle.process.internal.child.BootstrapSecurityManager -Dfile.encoding=UTF-8 -Duser.country=US -Duser.language=en -Duser.variant -ea -cp /root/.gradle/caches/2.1/workerMain/gradle-worker.jar jarjar.org.gradle.process.internal.launcher.GradleWorkerMain 'Gradle Test Executor 2' Successfully started process 'Gradle Test Executor 2' Gradle Test Executor 2 started executing tests. org.apache.bigtop.itest.hadoop.mapreduce.TestHadoopExamples STANDARD_ERROR log4j:WARN No appenders could be found for logger (java.lang.Object). log4j:WARN Please initialize the log4j system properly. log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info. > Building 73% > :mapreduce:tes let me know if this is okay to commit, would like to get it in asap so that the vagrant recipes in head are fixed. thanks!
          Hide
          cos Konstantin Boudnik added a comment -

          Two more things:

          • please document the use of this variable
            +  private Boolean useProperties = Boolean.parseBoolean(System.getProperty("useFailureProperties", "false"));
            
          • looks like unmatched versions of groovy here, no?
               testCompile module('org.codehaus.groovy:groovy:1.8.0')
            ...
            +  compile "org.codehaus.groovy:groovy-all:2.1.5"
            
          • these lines are misaligned
            +       srcDirs = [ 
            +         "$System.env.BIGTOP_HOME"+"/bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/",
            +	 "$System.env.BIGTOP_HOME"+"/bigtop-test-framework/src/test/groovy/org/apache/bigtop/itest/"
            +	]
            
          • unwarranted indentation change
            -          'conf/',
            +           'conf/'
            
          • indentation is wrong here as well
                   srcDirs = [
            -          "$System.env.BIGTOP_HOME"+"/bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/mapreduce"]
            +        "$System.env.BIGTOP_HOME"+"/bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/mapreduce",
            +        "$System.env.BIGTOP_HOME"+"/bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/",
            +	"$System.env.BIGTOP_HOME"+"/bigtop-test-framework/src/test/groovy/org/apache/bigtop/itest/"
            +      ]
            

            Thanks!

          Show
          cos Konstantin Boudnik added a comment - Two more things: please document the use of this variable + private Boolean useProperties = Boolean .parseBoolean( System .getProperty( "useFailureProperties" , " false " )); looks like unmatched versions of groovy here, no? testCompile module('org.codehaus.groovy:groovy:1.8.0') ... + compile "org.codehaus.groovy:groovy-all:2.1.5" these lines are misaligned + srcDirs = [ + "$ System .env.BIGTOP_HOME" + "/bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/" , + "$ System .env.BIGTOP_HOME" + "/bigtop-test-framework/src/test/groovy/org/apache/bigtop/itest/" + ] unwarranted indentation change - 'conf/', + 'conf/' indentation is wrong here as well srcDirs = [ - "$ System .env.BIGTOP_HOME" + "/bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/mapreduce" ] + "$ System .env.BIGTOP_HOME" + "/bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/mapreduce" , + "$ System .env.BIGTOP_HOME" + "/bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/" , + "$ System .env.BIGTOP_HOME" + "/bigtop-test-framework/src/test/groovy/org/apache/bigtop/itest/" + ] Thanks!
          Hide
          jayunit100 jay vyas added a comment - - edited

          okay here we go ! will update commit message when i push it if this looks good

          also ill update the integration test wiki on commit as well

          Show
          jayunit100 jay vyas added a comment - - edited okay here we go ! will update commit message when i push it if this looks good also ill update the integration test wiki on commit as well
          Hide
          cos Konstantin Boudnik added a comment -

          I still see
          these lines are misaligned

          +       srcDirs = [ 
          +         "$System.env.BIGTOP_HOME"+"/bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/",
          +	 "$System.env.BIGTOP_HOME"+"/bigtop-test-framework/src/test/groovy/org/apache/bigtop/itest/"
          +	]
          

          and what about the two different groovy versions? Is this correct?

          Show
          cos Konstantin Boudnik added a comment - I still see these lines are misaligned + srcDirs = [ + "$ System .env.BIGTOP_HOME" + "/bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/" , + "$ System .env.BIGTOP_HOME" + "/bigtop-test-framework/src/test/groovy/org/apache/bigtop/itest/" + ] and what about the two different groovy versions? Is this correct?
          Hide
          jayunit100 jay vyas added a comment -

          okay, reattached..

          1) sorry about that issue w alignment, fixed now, the tabs/spaces are acting funny on this box... better now?

          2) groovy's now look consistent to me ? 2.1.5?

          +  testCompile module('org.codehaus.groovy:groovy-all:2.1.5')
          +  compile "org.codehaus.groovy:groovy-all:2.1.5"
          
          Show
          jayunit100 jay vyas added a comment - okay, reattached.. 1) sorry about that issue w alignment, fixed now, the tabs/spaces are acting funny on this box... better now? 2) groovy's now look consistent to me ? 2.1.5? + testCompile module('org.codehaus.groovy:groovy-all:2.1.5') + compile "org.codehaus.groovy:groovy-all:2.1.5"
          Hide
          cos Konstantin Boudnik added a comment -

          Indents look ok, thank you!

          groovy-all:2.1.5

          The rest of the smoke tests are still using groovy 1.8.0. Unless you absolutely have to bump the version here, let's not do this as a part of the fix for FailureExecutor and bump it universally as a separate JIRA. ok?

          Show
          cos Konstantin Boudnik added a comment - Indents look ok, thank you! groovy-all:2.1.5 The rest of the smoke tests are still using groovy 1.8.0. Unless you absolutely have to bump the version here, let's not do this as a part of the fix for FailureExecutor and bump it universally as a separate JIRA. ok?
          Hide
          jayunit100 jay vyas added a comment - - edited

          ah i see what you mean. fixed and attached I better make that gradle a global property to prevent in the future. will make jira for that.

          looks good now?

          Show
          jayunit100 jay vyas added a comment - - edited ah i see what you mean. fixed and attached I better make that gradle a global property to prevent in the future. will make jira for that. looks good now?
          Hide
          cos Konstantin Boudnik added a comment -

          I think it should be
          + testCompile module('org.codehaus.groovy:groovy:1.8.0')
          not
          + testCompile module('org.codehaus.groovy:groovy-all:1.8.0')
          ??? At least the rest of gradle builds are doing that.

          Show
          cos Konstantin Boudnik added a comment - I think it should be + testCompile module('org.codehaus.groovy:groovy:1.8.0') not + testCompile module('org.codehaus.groovy:groovy-all:1.8.0') ??? At least the rest of gradle builds are doing that.
          Hide
          jayunit100 jay vyas added a comment - - edited

          ha, well next time i do a one off quickie patch ill think twice
          im getting good at rebasing.

          your right again cos. no need for gradle-all ,.... sorry am multitasking.

          reattached and tested as well

          Show
          jayunit100 jay vyas added a comment - - edited ha, well next time i do a one off quickie patch ill think twice im getting good at rebasing. your right again cos. no need for gradle-all ,.... sorry am multitasking. reattached and tested as well
          Hide
          cos Konstantin Boudnik added a comment -

          Thanks for finishing it up Jay! +1 let's commit it.
          I will look at BIGTOP-1520 tomorrow

          Show
          cos Konstantin Boudnik added a comment - Thanks for finishing it up Jay! +1 let's commit it. I will look at BIGTOP-1520 tomorrow
          Hide
          jayunit100 jay vyas added a comment -

          commited !

          Show
          jayunit100 jay vyas added a comment - commited !
          Hide
          cos Konstantin Boudnik added a comment -

          I have just noticed that the patch was dirly actually, and included the following irrelevant code in the patch:

          diff --git a/bigtop-deploy/vm/smoke-tests.sh b/bigtop-deploy/vm/smoke-tests.sh
          index cfb4050..abc0c09 100755
          --- a/bigtop-deploy/vm/smoke-tests.sh
          +++ b/bigtop-deploy/vm/smoke-tests.sh
          @@ -17,4 +17,4 @@ su -s /bin/bash $HCFS_USER -c 'hadoop fs -chmod 777 /user/vagrant'
           su -s /bin/bash $HCFS_USER -c 'hadoop fs -chmod 777 /user/root'
          

          We need to me more careful preparing patches (and perhaps reviewing them too). I will open a ticket to revert these lines (unless they are now required elsewhere). jay vyas, could you please take a look to make sure if these lines are needed for something else? Thanks!

          Show
          cos Konstantin Boudnik added a comment - I have just noticed that the patch was dirly actually, and included the following irrelevant code in the patch: diff --git a/bigtop-deploy/vm/smoke-tests.sh b/bigtop-deploy/vm/smoke-tests.sh index cfb4050..abc0c09 100755 --- a/bigtop-deploy/vm/smoke-tests.sh +++ b/bigtop-deploy/vm/smoke-tests.sh @@ -17,4 +17,4 @@ su -s /bin/bash $HCFS_USER -c 'hadoop fs -chmod 777 /user/vagrant' su -s /bin/bash $HCFS_USER -c 'hadoop fs -chmod 777 /user/root' We need to me more careful preparing patches (and perhaps reviewing them too). I will open a ticket to revert these lines (unless they are now required elsewhere). jay vyas , could you please take a look to make sure if these lines are needed for something else? Thanks!
          Hide
          jayunit100 jay vyas added a comment - - edited

          Hi Konstantin Boudnik . I think, this hunk looks good to me .... What it is doing is updating the vagrant tester to make sure it runs mapreduce tests.

          TL;DR this hunk is an update to the testing framework which reproduces the bug fixed in this patch, so that we dont regress.

          (details)

          The reason that is important (and most relevant to BIGTOP-1524) is because FailureVars broke the mapreduce tests, specifically.

          So this patch includes a update to the automated tests, that they run mapreduce as well as pig at a minimum when we test releases using vagrant.
          Makes sense now ? Or let me know if Im missing something.

          diff --git a/bigtop-deploy/vm/smoke-tests.sh b/bigtop-deploy/vm/smoke-tests.sh
          index cfb4050..abc0c09 100755
          --- a/bigtop-deploy/vm/smoke-tests.sh
          +++ b/bigtop-deploy/vm/smoke-tests.sh
          @@ -17,4 +17,4 @@ su -s /bin/bash $HCFS_USER -c 'hadoop fs -chmod 777 /user/vagrant'
           su -s /bin/bash $HCFS_USER -c 'hadoop fs -chmod 777 /user/root'
          
           yum install -y pig hive flume mahout
          -cd /bigtop-home/bigtop-tests/smoke-tests && ./gradlew compileGroovy test -Dsmoke.tests=pig --info
          +cd /bigtop-home/bigtop-tests/smoke-tests && ./gradlew clean compileGroovy test -Dsmoke.tests=mapreduce,pig --info
          
          Show
          jayunit100 jay vyas added a comment - - edited Hi Konstantin Boudnik . I think, this hunk looks good to me .... What it is doing is updating the vagrant tester to make sure it runs mapreduce tests. TL;DR this hunk is an update to the testing framework which reproduces the bug fixed in this patch, so that we dont regress . (details) The reason that is important (and most relevant to BIGTOP-1524 ) is because FailureVars broke the mapreduce tests, specifically. So this patch includes a update to the automated tests, that they run mapreduce as well as pig at a minimum when we test releases using vagrant. Makes sense now ? Or let me know if Im missing something. diff --git a/bigtop-deploy/vm/smoke-tests.sh b/bigtop-deploy/vm/smoke-tests.sh index cfb4050..abc0c09 100755 --- a/bigtop-deploy/vm/smoke-tests.sh +++ b/bigtop-deploy/vm/smoke-tests.sh @@ -17,4 +17,4 @@ su -s /bin/bash $HCFS_USER -c 'hadoop fs -chmod 777 /user/vagrant' su -s /bin/bash $HCFS_USER -c 'hadoop fs -chmod 777 /user/root' yum install -y pig hive flume mahout -cd /bigtop-home/bigtop-tests/smoke-tests && ./gradlew compileGroovy test -Dsmoke.tests=pig --info +cd /bigtop-home/bigtop-tests/smoke-tests && ./gradlew clean compileGroovy test -Dsmoke.tests=mapreduce,pig --info
          Hide
          cos Konstantin Boudnik added a comment -

          Yeah, it makes sense. It is just I am always got badly confused when I see different fixes mixed together. Arguably, improving the vagrant tests isn't relevant to the fix in question, hence my puzzlement.

          Show
          cos Konstantin Boudnik added a comment - Yeah, it makes sense. It is just I am always got badly confused when I see different fixes mixed together. Arguably, improving the vagrant tests isn't relevant to the fix in question, hence my puzzlement.
          Hide
          jayunit100 jay vyas added a comment - - edited

          improving the vagrant tests isn't relevant...

          okay, so im a little confused - b/c this patch is specifically about the vagrant tests. let me ask another way: for future, do you think that the vagrant updates should happen before submitting this patch or afterwards ? For me I think its a simultaneous thing ~ you would ideally want to have the tests commited along side the fix, commiting after doesnt help you verify that the fix is actually doing anything. Let me know, it could help me in the future w/ later JIRAs... (or possibly i should have just been more clear in the comment thread about what the vagrant modification was for)

          Show
          jayunit100 jay vyas added a comment - - edited improving the vagrant tests isn't relevant... okay, so im a little confused - b/c this patch is specifically about the vagrant tests. let me ask another way: for future, do you think that the vagrant updates should happen before submitting this patch or afterwards ? For me I think its a simultaneous thing ~ you would ideally want to have the tests commited along side the fix , commiting after doesnt help you verify that the fix is actually doing anything. Let me know, it could help me in the future w/ later JIRAs... (or possibly i should have just been more clear in the comment thread about what the vagrant modification was for)
          Hide
          cos Konstantin Boudnik added a comment -

          I am sorry Jay - you're right of course. I was under the impression that this is BIGTOP-1513. My bad - I shouldn't write comments at mid-night

          Show
          cos Konstantin Boudnik added a comment - I am sorry Jay - you're right of course. I was under the impression that this is BIGTOP-1513 . My bad - I shouldn't write comments at mid-night
          Hide
          jayunit100 jay vyas added a comment - - edited

          gotcha okay that makes more sense ! . Either way its good to be vigiilant and do one patch per feature , so i'll continue to keep it in mind !

          Show
          jayunit100 jay vyas added a comment - - edited gotcha okay that makes more sense ! . Either way its good to be vigiilant and do one patch per feature , so i'll continue to keep it in mind !

            People

            • Assignee:
              jayunit100 jay vyas
              Reporter:
              jayunit100 jay vyas
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development