Bug 92432 – nrwt: clean up --print options, logging modes

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dirk Pranke]

There's entirely too much complexity in printing/logging in nrwt. I've now got nearly (?) all of it stuffed behind the printing.py wall, so I can clean it up and eliminate a lot of the options.

My current plan is to make the output (and options) resemble what is currently implemented in test-webkitpy, with the following options:

--quiet : logs a ninja-esque status update, warnings, errors, tests that fail, and a one line summary at the end, *and nothing else*. If all tests pass successfully, you get one line of output indicating that.

(no args): equivalent to --print default,config today (more or less). Some introductory output logging the configuration, plus what you get with --quiet.

--verbose: the default plus one line per test, containing the results of the test

--details: --verbose plus the details of each test run a la --print trace-everything

--verbose --verbose: equivalent to today's --verbose: you get everything (except for --details -level information ; -vv -details would get that).

the --print argument will be removed.

comments welcome :).

[Dirk Pranke]

Created attachment 155693 [details]
Patch

[Dirk Pranke]

Okay, the attached patch does all of the above except for the ninja-style progress bar (which will come in a separate patch).

[Csaba Osztrogonác]

Now buildbots use --verbose (get from run-webkit-tests wrapper script).
We should modify it to --verbose --verbose not to change this behaviour.

(A little bit later I'll check all of these options.)

[Dirk Pranke]

discussion on webkit-dev@ suggests that we should use --debug instead of '-vv' or '--verbose --verbose'; however, we already use --debug to run the debug build (instead of the release build).

Ryosuke, what do you think of --debug-logging instead? 

We could also use something like --show-test-details (or --print-test-details) instead of --details, and --show-each-test instead of --verbose (possibly then leaving --verbose as it is today).

[Ryosuke Niwa]

(In reply to comment #4)
> discussion on webkit-dev@ suggests that we should use --debug instead of '-vv' or '--verbose --verbose'; however, we already use --debug to run the debug build (instead of the release build).
> 
> Ryosuke, what do you think of --debug-logging instead? 
> 
> We could also use something like --show-test-details (or --print-test-details) instead of --details, and --show-each-test instead of --verbose (possibly then leaving --verbose as it is today).

Ah, that's a good point. Maybe --more-verbose as John suggested on the thread?

[Alexey Proskuryakov]

This is all bikeshed-painting, but the reason to separate logging levels is that one is best for engineers working on WebKit, and another is best for debugging NRWT itself.

It's not about having more or less verbosity - it's entirely possible that we'll want some logging that is only usable in "less verbose" mode.

[Dirk Pranke]

(In reply to comment #6)
> This is all bikeshed-painting, but the reason to separate logging levels is that one is best for engineers working on WebKit, and another is best for debugging NRWT itself.
> 

This is true.

> It's not about having more or less verbosity - it's entirely possible that we'll want some logging that is only usable in "less verbose" mode.

I'm not quite sure what to make of this comment; are you suggesting we should have different names?

[Dirk Pranke]

I don't have a objection to --more-verbose or --extra-verbose, but they're kinda awkward :)

Any objection to making whatever we pick a synonym for '--verbose --verbose' so that that at least works, too? It would be useful at least temporarily since at least some of the bots are already passing it twice in preparation for something like this landing?

[Ryosuke Niwa]

(In reply to comment #8)
> I don't have a objection to --more-verbose or --extra-verbose, but they're kinda awkward :)
> 
> Any objection to making whatever we pick a synonym for '--verbose --verbose' so that that at least works, too? It would be useful at least temporarily since at least some of the bots are already passing it twice in preparation for something like this landing?

I object. Treating an option that appears for the second time differently from the first time is possibly the worst thing we can ever do in command line options. It's confusing and error prone.

[Ryosuke Niwa]

I'm fine with any of suggested names as long as it's separate from --verbose, --details, and other options.

[Dirk Pranke]

(In reply to comment #9)
> (In reply to comment #8)
> > I don't have a objection to --more-verbose or --extra-verbose, but they're kinda awkward :)
> > 
> > Any objection to making whatever we pick a synonym for '--verbose --verbose' so that that at least works, too? It would be useful at least temporarily since at least some of the bots are already passing it twice in preparation for something like this landing?
> 
> I object. Treating an option that appears for the second time differently from the first time is possibly the worst thing we can ever do in command line options. It's confusing and error prone.

Okay :).

[Dirk Pranke]

I think I'll probably go with --debug-nrwt-logging instead of --more-verbose or --extra-verbose, though. As Alexey says, it does help to make it clearer that this is about nrwt rather than the tests themselves.

[Alexey Proskuryakov]

> --debug-nrwt-logging

I agree with the direction of this discussion, but not with this name. "New" in NRWT is something we should get rid of sooner or later, and then the option would have to be renamed again. 

It's a poor practice to have "new" in tool or file names - "old" remains old, but "new" never remains new. I think that the time to remove "new" from nrwt is now.

[Dirk Pranke]

(In reply to comment #13)
> > --debug-nrwt-logging
> 
> I agree with the direction of this discussion, but not with this name. "New" in NRWT is something we should get rid of sooner or later, and then the option would have to be renamed again. 
> 
> It's a poor practice to have "new" in tool or file names - "old" remains old, but "new" never remains new. I think that the time to remove "new" from nrwt is now.

I actually had the same thought as you (about the undesirability of "nrwt" in the flag) but I didn't like "--debug-logging" because one might think it would include DRT logging, etc., and "--debug-rwt-logging" looked kinda goofy. Maybe it's not so bad, though? Any other suggestions?

[Ryosuke Niwa]

(In reply to comment #14)
> I actually had the same thought as you (about the undesirability of "nrwt" in the flag) but I didn't like "--debug-logging" because one might think it would include DRT logging, etc., and "--debug-rwt-logging" looked kinda goofy. Maybe it's not so bad, though? Any other suggestions?

--debug-rwt-logging sounds fine to me.

[Alexey Proskuryakov]

--debug-rwt-logging or --debug-rwt both seem fine to me.

[Dirk Pranke]

sold on --debug-rwt-logging!

[Dirk Pranke]

Created attachment 155936 [details]
rename -vv to --debug-rwt-logging

[Dirk Pranke]

okay, can haz review plz?

[Dirk Pranke]

Comment on attachment 155936 [details]
rename -vv to --debug-rwt-logging

okay, I'm gonna go on the assumption that this patch is too big to review easily and split it up a bit ...

[Ryosuke Niwa]

(In reply to comment #20)
> (From update of attachment 155936 [details])
> okay, I'm gonna go on the assumption that this patch is too big to review easily and split it up a bit ...

Yeah, that'll make the review easier. I did take a brief look at it but I decided that I need to read through it a couple of times as is.

[Dirk Pranke]

okay, first patch posted in bug 93018; I think there'll probably be three patches total (one more to clean up printing.py, and another to implement the new --verbose output).

[Dirk Pranke]

all patches have been landed, closing. I still need to change the progress meter to be a little more ninja-esque, but that'll be a separate bug.