Patchwork [resend] run-tests: make sure to check if pygments is installed before using it

login
register
mail settings
Submitter Pulkit Goyal
Date July 17, 2017, 6:37 p.m.
Message ID <37df34ca705cf68eef4f.1500316652@workspace>
Download mbox | patch
Permalink /patch/22451/
State Accepted
Headers show

Comments

Pulkit Goyal - July 17, 2017, 6:37 p.m.
# HG changeset patch
# User Pulkit Goyal <7895pulkit@gmail.com>
# Date 1500065225 -19800
#      Sat Jul 15 02:17:05 2017 +0530
# Node ID 37df34ca705cf68eef4fa6b4087eb039f775d4e6
# Parent  0353c051d54702a960e4efba1eea6fbc13ad401a
run-tests: make sure to check if pygments is installed before using it

e80041832e introduced support to color the output of tests but used pygments
without checking whether it's installed or not. That breaks test-run-tests.t for
machines which don't have pygments installed. This patch conditionalize the
color test in test-run-tests.t and also add a check to make sure pygments is
installed before using that.
Jun Wu - July 17, 2017, 7:24 p.m.
LGTM.

Excerpts from Pulkit Goyal's message of 2017-07-18 00:07:32 +0530:
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1500065225 -19800
> #      Sat Jul 15 02:17:05 2017 +0530
> # Node ID 37df34ca705cf68eef4fa6b4087eb039f775d4e6
> # Parent  0353c051d54702a960e4efba1eea6fbc13ad401a
> run-tests: make sure to check if pygments is installed before using it
> 
> e80041832e introduced support to color the output of tests but used pygments
> without checking whether it's installed or not. That breaks test-run-tests.t for
> machines which don't have pygments installed. This patch conditionalize the
> color test in test-run-tests.t and also add a check to make sure pygments is
> installed before using that.
> 
> diff --git a/tests/run-tests.py b/tests/run-tests.py
> --- a/tests/run-tests.py
> +++ b/tests/run-tests.py
> @@ -89,7 +89,7 @@
>  processlock = threading.Lock()
>  
>  with_color = False
> -
> +pygmentspresent = False
>  # ANSI color is unsupported prior to Windows 10
>  if os.name != 'nt':
>      try: # is pygments installed
> @@ -97,6 +97,7 @@
>          import pygments.lexers as lexers
>          import pygments.formatters as formatters
>          with_color = True
> +        pygmentspresent = True
>      except ImportError:
>          pass
>  
> @@ -1650,7 +1651,7 @@
>                  else:
>                      self.stream.write('\n')
>                      for line in lines:
> -                        if with_color:
> +                        if with_color and pygmentspresent:
>                              line = pygments.highlight(
>                                      line,
>                                      lexers.DiffLexer(),
> diff --git a/tests/test-run-tests.t b/tests/test-run-tests.t
> --- a/tests/test-run-tests.t
> +++ b/tests/test-run-tests.t
> @@ -121,7 +121,7 @@
>  
>  test diff colorisation
>  
> -#if no-windows
> +#if no-windows pygments
>    $ rt test-failure.t --color always
>    
>    \x1b[38;5;124m--- $TESTTMP/test-failure.t\x1b[39m (esc)
Augie Fackler - July 17, 2017, 8:07 p.m.
On Mon, Jul 17, 2017 at 12:24:26PM -0700, Jun Wu wrote:
> LGTM.

Queued, thanks.

>
> Excerpts from Pulkit Goyal's message of 2017-07-18 00:07:32 +0530:
> > # HG changeset patch
> > # User Pulkit Goyal <7895pulkit@gmail.com>
> > # Date 1500065225 -19800
> > #      Sat Jul 15 02:17:05 2017 +0530
> > # Node ID 37df34ca705cf68eef4fa6b4087eb039f775d4e6
> > # Parent  0353c051d54702a960e4efba1eea6fbc13ad401a
> > run-tests: make sure to check if pygments is installed before using it
> >
> > e80041832e introduced support to color the output of tests but used pygments
> > without checking whether it's installed or not. That breaks test-run-tests.t for
> > machines which don't have pygments installed. This patch conditionalize the
> > color test in test-run-tests.t and also add a check to make sure pygments is
> > installed before using that.
> >
> > diff --git a/tests/run-tests.py b/tests/run-tests.py
> > --- a/tests/run-tests.py
> > +++ b/tests/run-tests.py
> > @@ -89,7 +89,7 @@
> >  processlock = threading.Lock()
> >
> >  with_color = False
> > -
> > +pygmentspresent = False
> >  # ANSI color is unsupported prior to Windows 10
> >  if os.name != 'nt':
> >      try: # is pygments installed
> > @@ -97,6 +97,7 @@
> >          import pygments.lexers as lexers
> >          import pygments.formatters as formatters
> >          with_color = True
> > +        pygmentspresent = True
> >      except ImportError:
> >          pass
> >
> > @@ -1650,7 +1651,7 @@
> >                  else:
> >                      self.stream.write('\n')
> >                      for line in lines:
> > -                        if with_color:
> > +                        if with_color and pygmentspresent:
> >                              line = pygments.highlight(
> >                                      line,
> >                                      lexers.DiffLexer(),
> > diff --git a/tests/test-run-tests.t b/tests/test-run-tests.t
> > --- a/tests/test-run-tests.t
> > +++ b/tests/test-run-tests.t
> > @@ -121,7 +121,7 @@
> >
> >  test diff colorisation
> >
> > -#if no-windows
> > +#if no-windows pygments
> >    $ rt test-failure.t --color always
> >
> >    \x1b[38;5;124m--- $TESTTMP/test-failure.t\x1b[39m (esc)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - July 17, 2017, 10:17 p.m.
On Mon, Jul 17, 2017 at 1:07 PM, Augie Fackler <raf@durin42.com> wrote:
> On Mon, Jul 17, 2017 at 12:24:26PM -0700, Jun Wu wrote:
>> LGTM.
>
> Queued, thanks.
>
>>
>> Excerpts from Pulkit Goyal's message of 2017-07-18 00:07:32 +0530:
>> > # HG changeset patch
>> > # User Pulkit Goyal <7895pulkit@gmail.com>
>> > # Date 1500065225 -19800
>> > #      Sat Jul 15 02:17:05 2017 +0530
>> > # Node ID 37df34ca705cf68eef4fa6b4087eb039f775d4e6
>> > # Parent  0353c051d54702a960e4efba1eea6fbc13ad401a
>> > run-tests: make sure to check if pygments is installed before using it

This means that "run-tests.py --color=always" still gives no color. Do
the tests still fail if we take only the tests/test-run-tests.t part
of the patch?

>> >
>> > e80041832e introduced support to color the output of tests but used pygments
>> > without checking whether it's installed or not. That breaks test-run-tests.t for
>> > machines which don't have pygments installed. This patch conditionalize the
>> > color test in test-run-tests.t and also add a check to make sure pygments is
>> > installed before using that.
>> >
>> > diff --git a/tests/run-tests.py b/tests/run-tests.py
>> > --- a/tests/run-tests.py
>> > +++ b/tests/run-tests.py
>> > @@ -89,7 +89,7 @@
>> >  processlock = threading.Lock()
>> >
>> >  with_color = False
>> > -
>> > +pygmentspresent = False
>> >  # ANSI color is unsupported prior to Windows 10
>> >  if os.name != 'nt':
>> >      try: # is pygments installed
>> > @@ -97,6 +97,7 @@
>> >          import pygments.lexers as lexers
>> >          import pygments.formatters as formatters
>> >          with_color = True
>> > +        pygmentspresent = True
>> >      except ImportError:
>> >          pass
>> >
>> > @@ -1650,7 +1651,7 @@
>> >                  else:
>> >                      self.stream.write('\n')
>> >                      for line in lines:
>> > -                        if with_color:
>> > +                        if with_color and pygmentspresent:
>> >                              line = pygments.highlight(
>> >                                      line,
>> >                                      lexers.DiffLexer(),
>> > diff --git a/tests/test-run-tests.t b/tests/test-run-tests.t
>> > --- a/tests/test-run-tests.t
>> > +++ b/tests/test-run-tests.t
>> > @@ -121,7 +121,7 @@
>> >
>> >  test diff colorisation
>> >
>> > -#if no-windows
>> > +#if no-windows pygments
>> >    $ rt test-failure.t --color always
>> >
>> >    \x1b[38;5;124m--- $TESTTMP/test-failure.t\x1b[39m (esc)
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Jun Wu - July 17, 2017, 10:38 p.m.
Excerpts from Martin von Zweigbergk's message of 2017-07-17 15:17:40 -0700:
> On Mon, Jul 17, 2017 at 1:07 PM, Augie Fackler <raf@durin42.com> wrote:
> > On Mon, Jul 17, 2017 at 12:24:26PM -0700, Jun Wu wrote:
> >> LGTM.
> >
> > Queued, thanks.
> >
> >>
> >> Excerpts from Pulkit Goyal's message of 2017-07-18 00:07:32 +0530:
> >> > # HG changeset patch
> >> > # User Pulkit Goyal <7895pulkit@gmail.com>
> >> > # Date 1500065225 -19800
> >> > #      Sat Jul 15 02:17:05 2017 +0530
> >> > # Node ID 37df34ca705cf68eef4fa6b4087eb039f775d4e6
> >> > # Parent  0353c051d54702a960e4efba1eea6fbc13ad401a
> >> > run-tests: make sure to check if pygments is installed before using it
> 
> This means that "run-tests.py --color=always" still gives no color. Do
> the tests still fail if we take only the tests/test-run-tests.t part
> of the patch?

Good catch. I think --color=always prints warning or errors out is desirable
when pygements is not available. So taking only the .t part makes sense to
me.
via Mercurial-devel - July 17, 2017, 11:04 p.m.
On Mon, Jul 17, 2017 at 3:38 PM, Jun Wu <quark@fb.com> wrote:
> Excerpts from Martin von Zweigbergk's message of 2017-07-17 15:17:40 -0700:
>> On Mon, Jul 17, 2017 at 1:07 PM, Augie Fackler <raf@durin42.com> wrote:
>> > On Mon, Jul 17, 2017 at 12:24:26PM -0700, Jun Wu wrote:
>> >> LGTM.
>> >
>> > Queued, thanks.
>> >
>> >>
>> >> Excerpts from Pulkit Goyal's message of 2017-07-18 00:07:32 +0530:
>> >> > # HG changeset patch
>> >> > # User Pulkit Goyal <7895pulkit@gmail.com>
>> >> > # Date 1500065225 -19800
>> >> > #      Sat Jul 15 02:17:05 2017 +0530
>> >> > # Node ID 37df34ca705cf68eef4fa6b4087eb039f775d4e6
>> >> > # Parent  0353c051d54702a960e4efba1eea6fbc13ad401a
>> >> > run-tests: make sure to check if pygments is installed before using it
>>
>> This means that "run-tests.py --color=always" still gives no color. Do
>> the tests still fail if we take only the tests/test-run-tests.t part
>> of the patch?
>
> Good catch. I think --color=always prints warning or errors out is desirable
> when pygements is not available. So taking only the .t part makes sense to
> me.

I'll accept Pulkit's version so I don't have to mess with it without
his approval. I'll back out the run-tests.py changes in a followup
instead

Patch

diff --git a/tests/run-tests.py b/tests/run-tests.py
--- a/tests/run-tests.py
+++ b/tests/run-tests.py
@@ -89,7 +89,7 @@ 
 processlock = threading.Lock()
 
 with_color = False
-
+pygmentspresent = False
 # ANSI color is unsupported prior to Windows 10
 if os.name != 'nt':
     try: # is pygments installed
@@ -97,6 +97,7 @@ 
         import pygments.lexers as lexers
         import pygments.formatters as formatters
         with_color = True
+        pygmentspresent = True
     except ImportError:
         pass
 
@@ -1650,7 +1651,7 @@ 
                 else:
                     self.stream.write('\n')
                     for line in lines:
-                        if with_color:
+                        if with_color and pygmentspresent:
                             line = pygments.highlight(
                                     line,
                                     lexers.DiffLexer(),
diff --git a/tests/test-run-tests.t b/tests/test-run-tests.t
--- a/tests/test-run-tests.t
+++ b/tests/test-run-tests.t
@@ -121,7 +121,7 @@ 
 
 test diff colorisation
 
-#if no-windows
+#if no-windows pygments
   $ rt test-failure.t --color always
   
   \x1b[38;5;124m--- $TESTTMP/test-failure.t\x1b[39m (esc)