Patchwork [V2,stable] run-tests: fixes the '--interactive' option error

login
register
mail settings
Submitter Anurag Goel
Date June 12, 2014, 8:45 a.m.
Message ID <1affd22f1dc03beb4f26.1402562738@ubuntu.ubuntu-domain>
Download mbox | patch
Permalink /patch/4982/
State Superseded
Commit 7e14d026c4c4951b0feffedcf88a41056974e3b0
Headers show

Comments

Anurag Goel - June 12, 2014, 8:45 a.m.
# HG changeset patch
# User anuraggoel <anurag.dsps@gmail.com>
# Date 1402562430 -19800
#      Thu Jun 12 14:10:30 2014 +0530
# Node ID 1affd22f1dc03beb4f26544c142d1c8e4aa9feed
# Parent  3308ec584ade85bf7ddb3026c747aee912593038
run-tests: fixes the '--interactive' option error

This patch fixes a regression recently introduced by a refactoring.

Previously when failure occurs while testing with '--interactive' was enable,
it didn't prompt user by asking whether he wants to accept this failure changes
or not.

This was happening beacuse of the 'if' condition
if ret or not self._options.interactive or \
    not os.path.exists(test.errpath):

Everytime failure occurs, this condition gets true and returns back even
when '--interactive' is enabled. This condition don't led the function to
execute further, which consist the '--interactive' functionality.

Now, on failure with '--interactive' enabled, it prompts user whether he wants
to accepts failure changes or not.
If yes then test gets passed, else test gets failed.

On every failure, results gets stored in "self.failures.append((test, reason))"
But if failure changes accepted by user then test must get "pop out" from failed
test list.
Pierre-Yves David - June 12, 2014, 10:39 p.m.
On 06/12/2014 01:45 AM, Anurag Goel wrote:
> # HG changeset patch
> # User anuraggoel <anurag.dsps@gmail.com>
> # Date 1402562430 -19800
> #      Thu Jun 12 14:10:30 2014 +0530
> # Node ID 1affd22f1dc03beb4f26544c142d1c8e4aa9feed
> # Parent  3308ec584ade85bf7ddb3026c747aee912593038
> run-tests: fixes the '--interactive' option error

This, patches is still doing two different things. please split it.

> This patch fixes a regression recently introduced by a refactoring.
>
> Previously when failure occurs while testing with '--interactive' was enable,
> it didn't prompt user by asking whether he wants to accept this failure changes
> or not.
>
> This was happening beacuse of the 'if' condition
> if ret or not self._options.interactive or \
>      not os.path.exists(test.errpath):
>
> Everytime failure occurs, this condition gets true and returns back even
> when '--interactive' is enabled. This condition don't led the function to
> execute further, which consist the '--interactive' functionality.
>
> Now, on failure with '--interactive' enabled, it prompts user whether he wants
> to accepts failure changes or not.
> If yes then test gets passed, else test gets failed.
>
> On every failure, results gets stored in "self.failures.append((test, reason))"
> But if failure changes accepted by user then test must get "pop out" from failed
> test list.
>
> diff -r 3308ec584ade -r 1affd22f1dc0 tests/run-tests.py
> --- a/tests/run-tests.py	Thu Jun 12 03:20:28 2014 +0530
> +++ b/tests/run-tests.py	Thu Jun 12 14:10:30 2014 +0530
> @@ -466,7 +466,8 @@
>                   # the stack trace. This is for historical reasons and
>                   # this decision could be revisted in the future,
>                   # especially for PythonTest instances.
> -                result.addFailure(self, str(e))
> +                if result.addFailure(self, str(e)):
> +                    success = True
>               except Exception:
>                   result.addError(self, sys.exc_info())
>               else:
> @@ -1078,6 +1079,20 @@
>           else:
>               if not self._options.nodiff:
>                   self.stream.write('\nERROR: %s output changed\n' % test)
> +
> +            if self._options.interactive:
> +                iolock.acquire()
> +                self.stream.write('Accept this change? [n] ')
> +                answer = sys.stdin.readline().strip()
> +                iolock.release()
> +                if answer.lower() in ('y', 'yes'):
> +                    if test.name.endswith('.t'):
> +                        rename(test.errpath, test.path)
> +                    else:
> +                        rename(test.errpath, '%s.out' % test.path)
> +                    self.failures.pop()
> +                    return 1
> +
>               self.stream.write('!')
>
>       def addError(self, *args, **kwargs):
> @@ -1137,20 +1152,6 @@
>                       self.stream.write(line)
>                   self.stream.flush()
>
> -        if ret or not self._options.interactive or \
> -            not os.path.exists(test.errpath):
> -            return
> -
> -        iolock.acquire()
> -        print 'Accept this change? [n] ',
> -        answer = sys.stdin.readline().strip()
> -        iolock.release()
> -        if answer.lower() in ('y', 'yes'):
> -            if test.name.endswith('.t'):
> -                rename(test.errpath, test.path)
> -            else:
> -                rename(test.errpath, '%s.out' % test.path)
> -
>       def startTest(self, test):
>           super(TestResult, self).startTest(test)
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>

Patch

diff -r 3308ec584ade -r 1affd22f1dc0 tests/run-tests.py
--- a/tests/run-tests.py	Thu Jun 12 03:20:28 2014 +0530
+++ b/tests/run-tests.py	Thu Jun 12 14:10:30 2014 +0530
@@ -466,7 +466,8 @@ 
                 # the stack trace. This is for historical reasons and
                 # this decision could be revisted in the future,
                 # especially for PythonTest instances.
-                result.addFailure(self, str(e))
+                if result.addFailure(self, str(e)):
+                    success = True
             except Exception:
                 result.addError(self, sys.exc_info())
             else:
@@ -1078,6 +1079,20 @@ 
         else:
             if not self._options.nodiff:
                 self.stream.write('\nERROR: %s output changed\n' % test)
+
+            if self._options.interactive:
+                iolock.acquire()
+                self.stream.write('Accept this change? [n] ')
+                answer = sys.stdin.readline().strip()
+                iolock.release()
+                if answer.lower() in ('y', 'yes'):
+                    if test.name.endswith('.t'):
+                        rename(test.errpath, test.path)
+                    else:
+                        rename(test.errpath, '%s.out' % test.path)
+                    self.failures.pop()
+                    return 1
+
             self.stream.write('!')
 
     def addError(self, *args, **kwargs):
@@ -1137,20 +1152,6 @@ 
                     self.stream.write(line)
                 self.stream.flush()
 
-        if ret or not self._options.interactive or \
-            not os.path.exists(test.errpath):
-            return
-
-        iolock.acquire()
-        print 'Accept this change? [n] ',
-        answer = sys.stdin.readline().strip()
-        iolock.release()
-        if answer.lower() in ('y', 'yes'):
-            if test.name.endswith('.t'):
-                rename(test.errpath, test.path)
-            else:
-                rename(test.errpath, '%s.out' % test.path)
-
     def startTest(self, test):
         super(TestResult, self).startTest(test)