[PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty

[PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty

[Štěpán Němec]

When the output doesn't go to a terminal (typical case: log files),
the escape sequences are just noise.

Signed-off-by: Štěpán Němec <snemec@redhat.com>
---
 iptables-test.py | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/iptables-test.py b/iptables-test.py
index 90e07feed365..e8fc0c75a43e 100755
--- a/iptables-test.py
+++ b/iptables-test.py
@@ -32,22 +32,25 @@ EXTENSIONS_PATH = "extensions"
 LOGFILE="/tmp/iptables-test.log"
 log_file = None
 
+STDOUT_IS_TTY = sys.stdout.isatty()
 
-class Colors:
-    HEADER = '\033[95m'
-    BLUE = '\033[94m'
-    GREEN = '\033[92m'
-    YELLOW = '\033[93m'
-    RED = '\033[91m'
-    ENDC = '\033[0m'
+def maybe_colored(color, text):
+    terminal_sequences = {
+        'green': '\033[92m',
+        'red': '\033[91m',
+    }
+
+    return (
+        terminal_sequences[color] + text + '\033[0m' if STDOUT_IS_TTY else text
+    )
 
 
 def print_error(reason, filename=None, lineno=None):
     '''
     Prints an error with nice colors, indicating file and line number.
     '''
-    print(filename + ": " + Colors.RED + "ERROR" +
-        Colors.ENDC + ": line %d (%s)" % (lineno, reason))
+    print(filename + ": " + maybe_colored('red', "ERROR") +
+        ": line %d (%s)" % (lineno, reason))
 
 
 def delete_rule(iptables, rule, filename, lineno):
@@ -282,7 +285,7 @@ def run_test_file(filename, netns):
     if netns:
         execute_cmd("ip netns del ____iptables-container-test", filename, 0)
     if total_test_passed:
-        print(filename + ": " + Colors.GREEN + "OK" + Colors.ENDC)
+        print(filename + ": " + maybe_colored('green', "OK"))
 
     f.close()
     return tests, passed

base-commit: e438b9766fcc86d9847312ff05f1d1dac61acf1f
-- 
2.33.0

Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty

[Phil Sutter]

Hi,

On Thu, Sep 02, 2021 at 01:33:07PM +0200, Štěpán Němec wrote:
> When the output doesn't go to a terminal (typical case: log files),
> the escape sequences are just noise.
> 
> Signed-off-by: Štěpán Němec <snemec@redhat.com>

Acked-by: Phil Sutter <phil@nwl.cc>

With one minor nit:

> diff --git a/iptables-test.py b/iptables-test.py
> index 90e07feed365..e8fc0c75a43e 100755
> --- a/iptables-test.py
> +++ b/iptables-test.py
> @@ -32,22 +32,25 @@ EXTENSIONS_PATH = "extensions"
>  LOGFILE="/tmp/iptables-test.log"
>  log_file = None
>  
> +STDOUT_IS_TTY = sys.stdout.isatty()
>  
> -class Colors:
> -    HEADER = '\033[95m'
> -    BLUE = '\033[94m'
> -    GREEN = '\033[92m'
> -    YELLOW = '\033[93m'
> -    RED = '\033[91m'
> -    ENDC = '\033[0m'
> +def maybe_colored(color, text):
> +    terminal_sequences = {
> +        'green': '\033[92m',
> +        'red': '\033[91m',
> +    }
> +
> +    return (
> +        terminal_sequences[color] + text + '\033[0m' if STDOUT_IS_TTY else text
> +    )

I would "simplify" this into:

| if not sys.stdout.isatty():
| 	return text
| return terminal_sequences[color] + text + '\033[0m'

But what's beautiful in C might be ugly in Python and I'm blind to that
aspect. ;)

Cheers, Phil

Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty

[Štěpán Němec]

On Fri, 03 Sep 2021 14:52:50 +0200
Phil Sutter wrote:

> With one minor nit:
>
>> diff --git a/iptables-test.py b/iptables-test.py
>> index 90e07feed365..e8fc0c75a43e 100755
>> --- a/iptables-test.py
>> +++ b/iptables-test.py
>> @@ -32,22 +32,25 @@ EXTENSIONS_PATH = "extensions"
>>  LOGFILE="/tmp/iptables-test.log"
>>  log_file = None
>>  
>> +STDOUT_IS_TTY = sys.stdout.isatty()
>>  
>> -class Colors:
>> -    HEADER = '\033[95m'
>> -    BLUE = '\033[94m'
>> -    GREEN = '\033[92m'
>> -    YELLOW = '\033[93m'
>> -    RED = '\033[91m'
>> -    ENDC = '\033[0m'
>> +def maybe_colored(color, text):
>> +    terminal_sequences = {
>> +        'green': '\033[92m',
>> +        'red': '\033[91m',
>> +    }
>> +
>> +    return (
>> +        terminal_sequences[color] + text + '\033[0m' if STDOUT_IS_TTY else text
>> +    )
>
> I would "simplify" this into:
>
> | if not sys.stdout.isatty():
> | 	return text
> | return terminal_sequences[color] + text + '\033[0m'

...which could be further simplified by dropping 'not' and swapping the
two branches.

So there seem to be two things here: double return instead of
conditional expression, and calling the isatty method for every relevant
log line instead of once at the beginning.

I deliberately avoided the latter and I think I still prefer the
conditional expression to multiple return statements, too, but either
way should keep the escape sequences out of the log files and I don't
feel strongly about it.

Thanks,

  Štěpán

Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty

[Phil Sutter]

On Fri, Sep 03, 2021 at 04:44:41PM +0200, Štěpán Němec wrote:
> On Fri, 03 Sep 2021 14:52:50 +0200
> Phil Sutter wrote:
> 
> > With one minor nit:
> >
> >> diff --git a/iptables-test.py b/iptables-test.py
> >> index 90e07feed365..e8fc0c75a43e 100755
> >> --- a/iptables-test.py
> >> +++ b/iptables-test.py
> >> @@ -32,22 +32,25 @@ EXTENSIONS_PATH = "extensions"
> >>  LOGFILE="/tmp/iptables-test.log"
> >>  log_file = None
> >>  
> >> +STDOUT_IS_TTY = sys.stdout.isatty()
> >>  
> >> -class Colors:
> >> -    HEADER = '\033[95m'
> >> -    BLUE = '\033[94m'
> >> -    GREEN = '\033[92m'
> >> -    YELLOW = '\033[93m'
> >> -    RED = '\033[91m'
> >> -    ENDC = '\033[0m'
> >> +def maybe_colored(color, text):
> >> +    terminal_sequences = {
> >> +        'green': '\033[92m',
> >> +        'red': '\033[91m',
> >> +    }
> >> +
> >> +    return (
> >> +        terminal_sequences[color] + text + '\033[0m' if STDOUT_IS_TTY else text
> >> +    )
> >
> > I would "simplify" this into:
> >
> > | if not sys.stdout.isatty():
> > | 	return text
> > | return terminal_sequences[color] + text + '\033[0m'
> 
> ...which could be further simplified by dropping 'not' and swapping the
> two branches.

My change was mostly about reducing the long line, i.e. cosmetics. ;)

> So there seem to be two things here: double return instead of
> conditional expression, and calling the isatty method for every relevant
> log line instead of once at the beginning.
> 
> I deliberately avoided the latter and I think I still prefer the
> conditional expression to multiple return statements, too, but either
> way should keep the escape sequences out of the log files and I don't
> feel strongly about it.

Oh, you're right. I missed the fact about repeated isatty call, which is
certainly worth keeping. One other thing I just noticed, you're dropping
the other colors' definitions. Maybe worth keeping them? Also I'm not
too happy about the Python exception if an unknown color name is passed
to maybe_colored(). OTOH though it's just a test script and such bug is
easily identified.

Cheers, Phil

Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty

[Štěpán Němec]

On Fri, 03 Sep 2021 17:34:20 +0200
Phil Sutter wrote:

> On Fri, Sep 03, 2021 at 04:44:41PM +0200, Štěpán Němec wrote:
>> On Fri, 03 Sep 2021 14:52:50 +0200
>> Phil Sutter wrote:
>> 
>> > With one minor nit:
>> >
>> >> diff --git a/iptables-test.py b/iptables-test.py
>> >> index 90e07feed365..e8fc0c75a43e 100755
>> >> --- a/iptables-test.py
>> >> +++ b/iptables-test.py
>> >> @@ -32,22 +32,25 @@ EXTENSIONS_PATH = "extensions"
>> >>  LOGFILE="/tmp/iptables-test.log"
>> >>  log_file = None
>> >>  
>> >> +STDOUT_IS_TTY = sys.stdout.isatty()
>> >>  
>> >> -class Colors:
>> >> -    HEADER = '\033[95m'
>> >> -    BLUE = '\033[94m'
>> >> -    GREEN = '\033[92m'
>> >> -    YELLOW = '\033[93m'
>> >> -    RED = '\033[91m'
>> >> -    ENDC = '\033[0m'
>> >> +def maybe_colored(color, text):
>> >> +    terminal_sequences = {
>> >> +        'green': '\033[92m',
>> >> +        'red': '\033[91m',
>> >> +    }
>> >> +
>> >> +    return (
>> >> +        terminal_sequences[color] + text + '\033[0m' if STDOUT_IS_TTY else text
>> >> +    )
>> >
>> > I would "simplify" this into:
>> >
>> > | if not sys.stdout.isatty():
>> > | 	return text
>> > | return terminal_sequences[color] + text + '\033[0m'
>> 
>> ...which could be further simplified by dropping 'not' and swapping the
>> two branches.
>
> My change was mostly about reducing the long line, i.e. cosmetics. ;)

Ah, I see. I agree it's not the prettiest thing, but it's still in 80
columns (something that can't be said about a few other lines in the
script).

> One other thing I just noticed, you're dropping the other colors'
> definitions. Maybe worth keeping them?

Is it? I couldn't find anything but red and green ever being used since
2012 when the file was added.

> Also I'm not too happy about the Python exception if an unknown color
> name is passed to maybe_colored(). OTOH though it's just a test script
> and such bug is easily identified.

Yes, it's a helper function in a test script, not some kind of public
API. I don't see how maybe_colored('magenta', ...) is different in that
respect or more likely than print(Colors.MAGENTA + ...) previously.

-- 
Štěpán

Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty

[Phil Sutter]

Hi,

On Mon, Sep 06, 2021 at 11:04:38AM +0200, Štěpán Němec wrote:
> On Fri, 03 Sep 2021 17:34:20 +0200
> Phil Sutter wrote:
> 
> > On Fri, Sep 03, 2021 at 04:44:41PM +0200, Štěpán Němec wrote:
> >> On Fri, 03 Sep 2021 14:52:50 +0200
> >> Phil Sutter wrote:
> >> 
> >> > With one minor nit:
> >> >
> >> >> diff --git a/iptables-test.py b/iptables-test.py
> >> >> index 90e07feed365..e8fc0c75a43e 100755
> >> >> --- a/iptables-test.py
> >> >> +++ b/iptables-test.py
> >> >> @@ -32,22 +32,25 @@ EXTENSIONS_PATH = "extensions"
> >> >>  LOGFILE="/tmp/iptables-test.log"
> >> >>  log_file = None
> >> >>  
> >> >> +STDOUT_IS_TTY = sys.stdout.isatty()
> >> >>  
> >> >> -class Colors:
> >> >> -    HEADER = '\033[95m'
> >> >> -    BLUE = '\033[94m'
> >> >> -    GREEN = '\033[92m'
> >> >> -    YELLOW = '\033[93m'
> >> >> -    RED = '\033[91m'
> >> >> -    ENDC = '\033[0m'
> >> >> +def maybe_colored(color, text):
> >> >> +    terminal_sequences = {
> >> >> +        'green': '\033[92m',
> >> >> +        'red': '\033[91m',
> >> >> +    }
> >> >> +
> >> >> +    return (
> >> >> +        terminal_sequences[color] + text + '\033[0m' if STDOUT_IS_TTY else text
> >> >> +    )
> >> >
> >> > I would "simplify" this into:
> >> >
> >> > | if not sys.stdout.isatty():
> >> > | 	return text
> >> > | return terminal_sequences[color] + text + '\033[0m'
> >> 
> >> ...which could be further simplified by dropping 'not' and swapping the
> >> two branches.
> >
> > My change was mostly about reducing the long line, i.e. cosmetics. ;)
> 
> Ah, I see. I agree it's not the prettiest thing, but it's still in 80
> columns (something that can't be said about a few other lines in the
> script).
> 
> > One other thing I just noticed, you're dropping the other colors'
> > definitions. Maybe worth keeping them?
> 
> Is it? I couldn't find anything but red and green ever being used since
> 2012 when the file was added.
> 
> > Also I'm not too happy about the Python exception if an unknown color
> > name is passed to maybe_colored(). OTOH though it's just a test script
> > and such bug is easily identified.
> 
> Yes, it's a helper function in a test script, not some kind of public
> API. I don't see how maybe_colored('magenta', ...) is different in that
> respect or more likely than print(Colors.MAGENTA + ...) previously.

OK, fine. I'm convinced. :)

Applied, thanks!

Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty

[Štěpán Němec]

On Mon, 13 Sep 2021 17:05:33 +0200
Phil Sutter wrote:

> Applied, thanks!

Thank you.

I see that you've pushed your series including the change to print error
messages to stdout [1] in the meantime.

I don't have a strong opinion on whether output of a script whose
(only?) purpose is to print diagnostic messages should go to stdout or
stderr, but I do think that having the "ERROR"s go to stderr and "OK"s
go to stdout is more confusing than useful: was that really intentional?

As a side effect of that change, my patch will act funny depending on
which output stream is being redirected, too.

(I'm sorry I haven't pointed this out earlier; I just skimmed your
patches and didn't notice this until double checking the conflict/merge
with my patch now.)

-- 
Štěpán

[1]
https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210906163038.15381-4-phil@nwl.cc/

Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty

[Phil Sutter]

Hey,

On Tue, Sep 14, 2021 at 11:03:42AM +0200, Štěpán Němec wrote:
> On Mon, 13 Sep 2021 17:05:33 +0200
> Phil Sutter wrote:
> 
> > Applied, thanks!
> 
> Thank you.
> 
> I see that you've pushed your series including the change to print error
> messages to stdout [1] in the meantime.
> 
> I don't have a strong opinion on whether output of a script whose
> (only?) purpose is to print diagnostic messages should go to stdout or
> stderr, but I do think that having the "ERROR"s go to stderr and "OK"s
> go to stdout is more confusing than useful: was that really intentional?
> 
> As a side effect of that change, my patch will act funny depending on
> which output stream is being redirected, too.

Argh, you're right and I missed that.


Printing errors to stderr is useful to compare failing tests against an
expected set of failures - it is simply a task of comparing output on
stderr with a recorded one.

To not overcomplicate things, maybe the easiest fix would be to print
colors only if both stdout and stderr are a tty. What do you think?

Cheers, Phil

Re: [PATCH iptables] iptables-test.py: print with color escapes only when stdout isatty

[Štěpán Němec]

On Tue, 14 Sep 2021 13:25:16 +0200
Phil Sutter wrote:

> Printing errors to stderr is useful to compare failing tests against an
> expected set of failures - it is simply a task of comparing output on
> stderr with a recorded one.

I see. I'm still not sure the expected convenience factor (avoiding some
grep-like post processing? but couldn't you compare the combined output
just the same?) outweighs the weirdness / least surprise factor (having
two variations of the same diagnostic message split on two separate
output streams).

> To not overcomplicate things, maybe the easiest fix would be to print
> colors only if both stdout and stderr are a tty. What do you think?

Yes, if the split has to stay, I don't have a better suggestion.

Thanks,

  Štěpán