Last Comment Bug 776184 - JSRefTest: Fix --no-progress, tty discovery, and simplify ProgressBar's interface
: JSRefTest: Fix --no-progress, tty discovery, and simplify ProgressBar's inter...
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Terrence Cole [:terrence]
: general
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 741487
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-20 18:38 PDT by Terrence Cole [:terrence]
Modified: 2012-07-26 05:10 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0 (10.95 KB, patch)
2012-07-20 18:38 PDT, Terrence Cole [:terrence]
sphink: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-07-20 18:38:59 PDT
Created attachment 644552 [details] [diff] [review]
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:] 2012-07-24 19:30:59 PDT
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] 2012-07-25 10:39:46 PDT
(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] 2012-07-25 10:52:07 PDT
(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:] 2012-07-25 11:00:16 PDT
(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] 2012-07-25 11:05:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/56a19b273c2b
Comment 6 Ed Morley [:emorley] 2012-07-26 05:10:54 PDT
https://hg.mozilla.org/mozilla-central/rev/56a19b273c2b

Note You need to log in before you can comment on or make changes to this bug.