Bug 49934 – nrwt - add --print-config, clean up create_driver interface

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt - add --print-config, clean up create_driver interface

[Dirk Pranke]

Created attachment 74590 [details]
Patch

[Dirk Pranke]

Note that this change is *not* related to the multiprocessing stuff, it's just that the cleanup now makes it a little more practical.

[Tony Chang]

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?

[Dirk Pranke]

(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.

[Tony Chang]

(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.

[Dirk Pranke]

(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".

[Dirk Pranke]

Created attachment 74802 [details]
update w/ review feedback, rebase to tip of tree

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 74806 [details]
remove --print-config; we'll add a separate --dry-run flag instead

[Dirk Pranke]

Created attachment 74807 [details]
actually pass the tests

[Tony Chang]

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

[Dirk Pranke]

--dry-run patch uploaded as bug 50043.

[Dirk Pranke]

(In reply to comment #12)
> --dry-run patch uploaded as bug 50043.

Make that bug 50045.

[Dirk Pranke]

Closed, landed as r72708.