Bug 49934 - nrwt - add --print-config, clean up create_driver interface
Summary: nrwt - add --print-config, clean up create_driver interface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-22 13:32 PST by Dirk Pranke
Modified: 2010-11-24 17:00 PST (History)
3 users (show)

See Also:


Attachments
Patch (28.88 KB, patch)
2010-11-22 13:36 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ review feedback, rebase to tip of tree (29.30 KB, patch)
2010-11-24 15:29 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
remove --print-config; we'll add a separate --dry-run flag instead (27.33 KB, patch)
2010-11-24 16:19 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
actually pass the tests (26.99 KB, patch)
2010-11-24 16:27 PST, Dirk Pranke
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2010-11-22 13:32:23 PST
nrwt - add --print-config, clean up create_driver interface
Comment 1 Dirk Pranke 2010-11-22 13:36:20 PST
Created attachment 74590 [details]
Patch
Comment 2 Dirk Pranke 2010-11-22 14:57:07 PST
Note that this change is *not* related to the multiprocessing stuff, it's just that the cleanup now makes it a little more practical.
Comment 3 Tony Chang 2010-11-22 17:53:32 PST
Comment on attachment 74590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74590&action=review

Some small clarifications . . .

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:123
> +        optparse.make_option("--print-config", action="store_true",
> +            default=False, help="Print the config used for the run and exit."),

Is the expectation to only use this during tests?  It would be nice to not expose the flag in --help if we don't have to.

> WebKitTools/Scripts/webkitpy/layout_tests/port/port_testcase.py:48
> +    def test_driver_cmd_line(self):
> +        port = self.make_port()

It looks like this test is only run by mac_unittest.py.  Would it be better to just move this test into that file and do more validation on the command line than len > 0?
Comment 4 Dirk Pranke 2010-11-22 18:18:29 PST
(In reply to comment #3)
> (From update of attachment 74590 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74590&action=review
> 
> Some small clarifications . . .
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:123
> > +        optparse.make_option("--print-config", action="store_true",
> > +            default=False, help="Print the config used for the run and exit."),
> 
> Is the expectation to only use this during tests?  It would be nice to not expose the flag in --help if we don't have to.
> 

No. This is very much intended to be a user-visible feature. I'm adding it because people have asked for an easy way to see what command line we're running with.

> > WebKitTools/Scripts/webkitpy/layout_tests/port/port_testcase.py:48
> > +    def test_driver_cmd_line(self):
> > +        port = self.make_port()
> 
> It looks like this test is only run by mac_unittest.py.  Would it be better to just move this test into that file and do more validation on the command line than len > 0?

That is not intentionally the case. I am fairly positive that when I originally wrote this more than one port was using it - I think I had a win_unittest.py as well, but maybe that never got checked in. At any rate, all of the port unittests should be subclassing this class and running these tests, at which point I can't assume anything too helpful about the command line. I can file a separate bug for that or add a FIXME or something.
Comment 5 Tony Chang 2010-11-23 11:43:35 PST
(In reply to comment #4)
> (In reply to comment #3)
> > Is the expectation to only use this during tests?  It would be nice to not expose the flag in --help if we don't have to.
> > 
> 
> No. This is very much intended to be a user-visible feature. I'm adding it because people have asked for an easy way to see what command line we're running with.

Is printing it when running (added in this patch) not sufficient?  It's pretty easy to run NRWT and press ctrl+c to get the command line then.  I only bring it up because NRWT --help is so long and unorganized that it's pretty much worthless now.  webkit-patch is much better in this regard.


> > > WebKitTools/Scripts/webkitpy/layout_tests/port/port_testcase.py:48
> > > +    def test_driver_cmd_line(self):
> > > +        port = self.make_port()
> > 
> > It looks like this test is only run by mac_unittest.py.  Would it be better to just move this test into that file and do more validation on the command line than len > 0?
> 
> That is not intentionally the case. I am fairly positive that when I originally wrote this more than one port was using it - I think I had a win_unittest.py as well, but maybe that never got checked in. At any rate, all of the port unittests should be subclassing this class and running these tests, at which point I can't assume anything too helpful about the command line. I can file a separate bug for that or add a FIXME or something.

Please file a separate bug and add a FIXME.  In general, I'm not in favor of abstraction that isn't used so it should either be used (more port unittests checked in) or removed soon.
Comment 6 Dirk Pranke 2010-11-23 11:52:51 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Is the expectation to only use this during tests?  It would be nice to not expose the flag in --help if we don't have to.
> > > 
> > 
> > No. This is very much intended to be a user-visible feature. I'm adding it because people have asked for an easy way to see what command line we're running with.
> 
> Is printing it when running (added in this patch) not sufficient?  It's pretty easy to run NRWT and press ctrl+c to get the command line then.  I only bring it up because NRWT --help is so long and unorganized that it's pretty much worthless now.  webkit-patch is much better in this regard.
> 

I personally have often wanted to be able to just verify the config and exit, and I don't really like having to ctrl-C things. Also, the alternatives are either remembering to do "--print config" which is just as bad as having "--print-config" (although at least it already exists) or "--verbose", in which case the message will likely be lost. I'd prefer the current approach.

I agree the --help output is long and confusing. There's a separate bug filed for cleaning up the option processing. I think rolling better printing into that would be fitting.

> 
> Please file a separate bug and add a FIXME.  In general, I'm not in favor of abstraction that isn't used so it should either be used (more port unittests checked in) or removed soon.

Will do. I think the abstraction is useful even as is, since it represents "tests that should be true on every port" as opposed to "mac-specific tests".
Comment 7 Dirk Pranke 2010-11-24 15:29:39 PST
Created attachment 74802 [details]
update w/ review feedback, rebase to tip of tree
Comment 8 Dirk Pranke 2010-11-24 15:31:16 PST
(In reply to comment #6)
> > 
> > Please file a separate bug and add a FIXME.  In general, I'm not in favor of abstraction that isn't used so it should either be used (more port unittests checked in) or removed soon.
> 
> Will do. I think the abstraction is useful even as is, since it represents "tests that should be true on every port" as opposed to "mac-specific tests".

FIXME added, bug 50043 filed and patch uploaded in order to not conflate things in this patch.
Comment 9 Dirk Pranke 2010-11-24 16:19:23 PST
Created attachment 74806 [details]
remove --print-config; we'll add a separate --dry-run flag instead
Comment 10 Dirk Pranke 2010-11-24 16:27:34 PST
Created attachment 74807 [details]
actually pass the tests
Comment 11 Tony Chang 2010-11-24 16:31:09 PST
Comment on attachment 74807 [details]
actually pass the tests

View in context: https://bugs.webkit.org/attachment.cgi?id=74807&action=review

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:538
>          if (not self._driver or self._driver.poll() is not None):

Nit: remove the ()s
Comment 12 Dirk Pranke 2010-11-24 16:51:12 PST
--dry-run patch uploaded as bug 50043.
Comment 13 Dirk Pranke 2010-11-24 16:53:53 PST
(In reply to comment #12)
> --dry-run patch uploaded as bug 50043.

Make that bug 50045.
Comment 14 Dirk Pranke 2010-11-24 17:00:28 PST
Closed, landed as r72708.