Summary: | nrwt: clean up --print options, logging modes | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||
Component: | Tools / Tests | Assignee: | 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
Dirk Pranke
2012-07-26 15:56:47 PDT
Created attachment 155693 [details]
Patch
Okay, the attached patch does all of the above except for the ninja-style progress bar (which will come in a separate patch). 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.) 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). (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? 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. (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? 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? (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. I'm fine with any of suggested names as long as it's separate from --verbose, --details, and other options. (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 :). 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. > --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.
(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? (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. --debug-rwt-logging or --debug-rwt both seem fine to me. sold on --debug-rwt-logging! Created attachment 155936 [details]
rename -vv to --debug-rwt-logging
okay, can haz review plz? 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 ...
(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. 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). 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. |