RESOLVED FIXED 92432
nrwt: clean up --print options, logging modes 
https://bugs.webkit.org/show_bug.cgi?id=92432
Summary nrwt: clean up --print options, logging modes
Dirk Pranke
Reported 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 :).
Attachments
Patch (70.20 KB, patch)
2012-07-31 18:05 PDT, Dirk Pranke 		no flags
DetailsFormatted DiffDiff
rename -vv to --debug-rwt-logging (70.68 KB, patch)
2012-08-01 17:52 PDT, Dirk Pranke 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Dirk Pranke
Comment 1 2012-07-31 18:05:35 PDT
Created attachment 155693 [details] Patch
Dirk Pranke
Comment 2 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).
Csaba Osztrogonác
Comment 3 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.)
Dirk Pranke
Comment 4 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).
Ryosuke Niwa
Comment 5 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?
Alexey Proskuryakov
Comment 6 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.
Dirk Pranke
Comment 7 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?
Dirk Pranke
Comment 8 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?
Ryosuke Niwa
Comment 9 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.
Ryosuke Niwa
Comment 10 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.
Dirk Pranke
Comment 11 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 :).
Dirk Pranke
Comment 12 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.
Alexey Proskuryakov
Comment 13 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.
Dirk Pranke
Comment 14 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?
Ryosuke Niwa
Comment 15 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.
Alexey Proskuryakov
Comment 16 2012-08-01 13:40:50 PDT
--debug-rwt-logging or --debug-rwt both seem fine to me.
Dirk Pranke
Comment 17 2012-08-01 13:43:43 PDT
sold on --debug-rwt-logging!
Dirk Pranke
Comment 18 2012-08-01 17:52:32 PDT
Created attachment 155936 [details] rename -vv to --debug-rwt-logging
Dirk Pranke
Comment 19 2012-08-01 17:53:28 PDT
okay, can haz review plz?
Dirk Pranke
Comment 20 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 ...
Ryosuke Niwa
Comment 21 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.
Dirk Pranke
Comment 22 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).
Dirk Pranke
Comment 23 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.
Note You need to log in before you can comment on or make changes to this bug.