Closed Bug 776184 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: terrence, Assigned: terrence)

References

Details

(Whiteboard: [js:t])

Attachments

(1 file)

Attached patch v0Splinter Review
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.
Attachment #644552 - Flags: review?(dmandelin)
Whiteboard: [js:t]
Attachment #644552 - Flags: review?(dmandelin) → review?(sphink)
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. ;)
Attachment #644552 - Flags: review?(sphink) → review+
(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. ;)

:-)
(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.
(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. :-)
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/56a19b273c2b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.