776184 - JSRefTest: Fix --no-progress, tty discovery, and simplify ProgressBar's interface

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

This applies on top of the patch in 741487.  It does what's on the tin and adds color, since our tty discovery is now the same as what we use in jit-tests.

Comment 1

[Steve Fink [:sfink] [:s:]]

Comment on attachment 644552 [details] [diff] [review]
v0

Review of attachment 644552 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/lib/progressbar.py
@@ +34,5 @@
> +        for layout in self.counters_fmt:
> +            self.counters_width += 4 # Less than 9999 tests total.
> +            self.counters_width += 1 # | (or ']' for the last one)
> +
> +        self.barlen = 64 - self.counters_width

Badnasty constant 64. Could it default to (a function of) $COLUMNS if set, and be given a name?

@@ +78,5 @@
> +        sys.stdout.write(msg)
> +        sys.stdout.write('\n')
> +
> +    @staticmethod
> +    def stdio_might_be_broken():

Not sure if you copied this in from somewhere else, but the name seems strange. I don't entirely follow what this is doing, but conservative_isatty()? safe_to_use_tty()?

The comment says "using stdio", but even the NullProgressBar uses sys.stdout, so I'm unclear on what this is testing.

@@ +89,5 @@
> +        try:
> +            import android
> +        except ImportError:
> +            return False
> +        return True and sys.stdout.isatty()

Seems remarkably similar to

  return sys.stdout.isatty()

::: js/src/tests/lib/results.py
@@ -87,5 @@
>          self.n = 0
>  
> -        self.pb = None
> -        if not options.hide_progress:
> -            self.pb = ProgressBar('', testcount, 21)

Ah! The magic number I complained about in the other patch is dead. Please retroactively ignore that comment. ;)

Comment 2

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink] from comment #1)
> ::: js/src/tests/lib/progressbar.py
> @@ +34,5 @@
> > +        for layout in self.counters_fmt:
> > +            self.counters_width += 4 # Less than 9999 tests total.
> > +            self.counters_width += 1 # | (or ']' for the last one)
> > +
> > +        self.barlen = 64 - self.counters_width
> 
> Badnasty constant 64. Could it default to (a function of) $COLUMNS if set,
> and be given a name?

I did not know about $COLUMNS, thank you for that.  Why is there no good introduction on the internet to terminal programming?  I've pretty much had to teach myself everything by reading Wikipedia and existing terminal programs.
 
> @@ +78,5 @@
> > +        sys.stdout.write(msg)
> > +        sys.stdout.write('\n')
> > +
> > +    @staticmethod
> > +    def stdio_might_be_broken():
> 
> Not sure if you copied this in from somewhere else, but the name seems
> strange. I don't entirely follow what this is doing, but
> conservative_isatty()? safe_to_use_tty()?

Duped from the jit-tests harness.  I like your name better.

> The comment says "using stdio", but even the NullProgressBar uses
> sys.stdout, so I'm unclear on what this is testing.

Yeah, that's pretty unclear.  I have changed it to.

"""
Prefer erring on the side of caution and not using terminal commands if
the current output stream may be a file.  We explicitly check for the
Android platform because terminal commands work poorly over ADB's
redirection.
"""

> @@ +89,5 @@
> > +        try:
> > +            import android
> > +        except ImportError:
> > +            return False
> > +        return True and sys.stdout.isatty()
> 
> Seems remarkably similar to
> 
>   return sys.stdout.isatty()

In *my* mind, the condition I am checking is: "is it android" or "is it a tty".  I'm just glad I didn't put the isatty() test on both returns.

> ::: js/src/tests/lib/results.py
> @@ -87,5 @@
> >          self.n = 0
> >  
> > -        self.pb = None
> > -        if not options.hide_progress:
> > -            self.pb = ProgressBar('', testcount, 21)
> 
> Ah! The magic number I complained about in the other patch is dead. Please
> retroactively ignore that comment. ;)

:-)

Comment 3

[Terrence Cole [:terrence]]

(In reply to Terrence Cole [:terrence] from comment #2)
> (In reply to Steve Fink [:sfink] from comment #1)
> > ::: js/src/tests/lib/progressbar.py
> > @@ +34,5 @@
> > > +        for layout in self.counters_fmt:
> > > +            self.counters_width += 4 # Less than 9999 tests total.
> > > +            self.counters_width += 1 # | (or ']' for the last one)
> > > +
> > > +        self.barlen = 64 - self.counters_width
> > 
> > Badnasty constant 64. Could it default to (a function of) $COLUMNS if set,
> > and be given a name?
> 
> I did not know about $COLUMNS, thank you for that.  Why is there no good
> introduction on the internet to terminal programming?  I've pretty much had
> to teach myself everything by reading Wikipedia and existing terminal
> programs.
  
COLUMNS is a local in bash.  It looks like one actually has to use terminal magic to get the width, so I've opened up a different bug for this: bug 777424.

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to Terrence Cole [:terrence] from comment #2)
> (In reply to Steve Fink [:sfink] from comment #1)
> > ::: js/src/tests/lib/progressbar.py
> > @@ +34,5 @@
> > > +        for layout in self.counters_fmt:
> > > +            self.counters_width += 4 # Less than 9999 tests total.
> > > +            self.counters_width += 1 # | (or ']' for the last one)
> > > +
> > > +        self.barlen = 64 - self.counters_width
> > 
> > Badnasty constant 64. Could it default to (a function of) $COLUMNS if set,
> > and be given a name?
> 
> I did not know about $COLUMNS, thank you for that.  Why is there no good
> introduction on the internet to terminal programming?  I've pretty much had
> to teach myself everything by reading Wikipedia and existing terminal
> programs.

Maybe because there's no good way to *do* terminal programming, from what I can tell. :-)

I found $COLUMNS by looking at what hg's progress bar does. It's fancier; it first tries $COLUMNS and failing that, does a bunch of platform-specific magic that on Posix boils down to

                arri = fcntl.ioctl(fd, termios.TIOCGWINSZ, '\0' * 8)
                width = array.array('h', arri)[1]
                if width > 0:
                    return width

Win32 is nuts in a different way:

    # cmd.exe does not handle CR like a unix console, the CR is
    # counted in the line length. On 80 columns consoles, if 80
    # characters are written, the following CR won't apply on the
    # current line but on the new one. Keep room for it.

So, like I said, try int(os.environ['COLUMNS']) and give up if the shell didn't helpfully provide it. :-)

Comment 5

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56a19b273c2b

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/56a19b273c2b