Bug 92432

Summary: nrwt: clean up --print options, logging modes
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: Tools / TestsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, eric, ojan, ossy, rniwa, tony
Priority: P2 Keywords: NRWT
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 88702, 93018, 93026    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
rename -vv to --debug-rwt-logging none

Description Dirk Pranke 2012-07-26 15:56:47 PDT
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 :).
Comment 1 Dirk Pranke 2012-07-31 18:05:35 PDT
Created attachment 155693 [details]
Patch
Comment 2 Dirk Pranke 2012-07-31 18:06:27 PDT
Okay, the attached patch does all of the above except for the ninja-style progress bar (which will come in a separate patch).
Comment 3 Csaba Osztrogonác 2012-07-31 23:28:57 PDT
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.)
Comment 4 Dirk Pranke 2012-08-01 11:08:34 PDT
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).
Comment 5 Ryosuke Niwa 2012-08-01 11:22:44 PDT
(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?
Comment 6 Alexey Proskuryakov 2012-08-01 11:27:44 PDT
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.
Comment 7 Dirk Pranke 2012-08-01 11:32:18 PDT
(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?
Comment 8 Dirk Pranke 2012-08-01 11:34:07 PDT
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?
Comment 9 Ryosuke Niwa 2012-08-01 11:37:59 PDT
(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.
Comment 10 Ryosuke Niwa 2012-08-01 11:38:46 PDT
I'm fine with any of suggested names as long as it's separate from --verbose, --details, and other options.
Comment 11 Dirk Pranke 2012-08-01 11:43:11 PDT
(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 :).
Comment 12 Dirk Pranke 2012-08-01 11:44:17 PDT
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.
Comment 13 Alexey Proskuryakov 2012-08-01 13:01:37 PDT
> --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.
Comment 14 Dirk Pranke 2012-08-01 13:18:26 PDT
(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?
Comment 15 Ryosuke Niwa 2012-08-01 13:19:53 PDT
(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.
Comment 16 Alexey Proskuryakov 2012-08-01 13:40:50 PDT
--debug-rwt-logging or --debug-rwt both seem fine to me.
Comment 17 Dirk Pranke 2012-08-01 13:43:43 PDT
sold on --debug-rwt-logging!
Comment 18 Dirk Pranke 2012-08-01 17:52:32 PDT
Created attachment 155936 [details]
rename -vv to --debug-rwt-logging
Comment 19 Dirk Pranke 2012-08-01 17:53:28 PDT
okay, can haz review plz?
Comment 20 Dirk Pranke 2012-08-02 11:32:42 PDT
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 ...
Comment 21 Ryosuke Niwa 2012-08-02 11:57:53 PDT
(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.
Comment 22 Dirk Pranke 2012-08-02 12:40:47 PDT
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).
Comment 23 Dirk Pranke 2012-08-07 19:11:37 PDT
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.