Bug 93233 – [Chromium]duplicated command line options in Android LayoutTest

• Component: New Bugs
• Status: RESOLVED FIXED

[Wei James (wistoch)]

[Chromium]duplicated command line options in Android LayoutTest

[Wei James (wistoch)]

Created attachment 156622 [details]
Patch

[WebKit Review Bot]

Attachment 156622 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
Tools/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wei James (wistoch)]

Created attachment 156626 [details]
Patch

[Tony Chang]

Comment on attachment 156626 [details]
Patch

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

Seems fine, but should we add a unit test for this?

> Tools/ChangeLog:12
> +        If there are multiple ChromiumAndroidPort instances, these two options
> +        will be appended for multiple times.

Do we normally have multiple ChromiumAndroidPort instances?  Does having these options multiple times cause anything to break?

[Wei James (wistoch)]

(In reply to comment #4)
> (From update of attachment 156626 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156626&action=review
> 
> Seems fine, but should we add a unit test for this?
> 
ok. I will add an unit test for it. 
> > Tools/ChangeLog:12
> > +        If there are multiple ChromiumAndroidPort instances, these two options
> > +        will be appended for multiple times.
> 
> Do we normally have multiple ChromiumAndroidPort instances?  Does having these options multiple times cause anything to break?

when I tried chromium android layout test, there are actually two ChromiumAndroidPort instances. So the command line contains two copies of "--encode-binary" and "--enable-hardware-gpu". 

I have not found anything broken by it but just feel odd to have multiple same options in the file chrome-native-tests-command-line.

[Xianzhu Wang]

Comment on attachment 156626 [details]
Patch

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

>>> Tools/ChangeLog:12
>>> +        will be appended for multiple times.
>> 
>> Do we normally have multiple ChromiumAndroidPort instances?  Does having these options multiple times cause anything to break?
> 
> when I tried chromium android layout test, there are actually two ChromiumAndroidPort instances. So the command line contains two copies of "--encode-binary" and "--enable-hardware-gpu". 
> 
> I have not found anything broken by it but just feel odd to have multiple same options in the file chrome-native-tests-command-line.

Just curious, do you know where the two instances are created and why they share the same self._options.additional_drt_flags?

[Wei James (wistoch)]

(In reply to comment #6)
> (From update of attachment 156626 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156626&action=review
> 
> >>> Tools/ChangeLog:12
> >>> +        will be appended for multiple times.
> >> 
> >> Do we normally have multiple ChromiumAndroidPort instances?  Does having these options multiple times cause anything to break?
> > 
> > when I tried chromium android layout test, there are actually two ChromiumAndroidPort instances. So the command line contains two copies of "--encode-binary" and "--enable-hardware-gpu". 
> > 
> > I have not found anything broken by it but just feel odd to have multiple same options in the file chrome-native-tests-command-line.
> 
> Just curious, do you know where the two instances are created and why they share the same self._options.additional_drt_flags?

the call stack of the first one:
1. run_webkit_tests.py line 468, in main
   port = host.port_factory.get(options.platform, options)
2. factory.py line 114  
   return cls(self._host, port_name, options=options, **kwargs)
3. chromium_android.py __init__

the call stack of the second one:
1. run_webkit_tests.py line 110, in run
      unexpected_result_count = manager.run(args)
2. manager.py line 418, in run
      self._run_tests(self._test_names, result_summary, int(self._options.child_processes))
3. manager.py line 471, in _run_tests
      return self._runner.run_tests(test_inputs, self._expectations, result_summary, num_workers, needs_http, needs_websockets, self._retrying)
4. layout_test_runner.py line 140, in run_tests
      pool.run(('test_list', shard.name, shard.test_inputs) for shard in all_shards)
5. message_pool.py line 97, in run
      self.wait()
6. message_pool.py line 127, in wait
      self._workers[0].run()
7. message_pool.py line 267, in run
      self._raise(sys.exc_info())
8. message_pool.py line 251, in run
      worker.start()
9. layout_test_runner.py line 293, in start
      self._port = self._host.port_factory.get(self._options.platform, self._options)
10. factory.py line 114, in get
      return cls(self._host, port_name, options=options, **kwargs)
11. chromium_android.py  __init__


they have the same options argument from main() of run_webkit_tests.py.

[Wei James (wistoch)]

will it make more sense to make the port instance singleton? 

that is, in factory.py, get() function can store the instance and return it when others call get() for the instance for the same port-name. 

but I don't know whether such chagne will break anything.

[Tony Chang]

Another approach would be to not mutate options.additional_drt_flag.  E.g., in driver.py's cmd_line, in addition to using _port.get_option('aditional_drt_flag', ...), we could add a _port.additional_drt_flag() method.

Dirk, thoughts?

[Dirk Pranke]

(In reply to comment #9)
> Another approach would be to not mutate options.additional_drt_flag.  E.g., in driver.py's cmd_line, in addition to using _port.get_option('aditional_drt_flag', ...), we could add a _port.additional_drt_flag() method.
> 
> Dirk, thoughts?

That might be slightly better. It's probably a flaw to be mutating the options argument in the port classes, but the assumption that we can do that pervades the code at the moment.

[Wei James (wistoch)]

Created attachment 156848 [details]
Patch

[Wei James (wistoch)]

tony, 

I have modified the patch to use your approach and added an unit test.

could you help to review the new patch? thanks

[Tony Chang]

Comment on attachment 156848 [details]
Patch

Can you rebase your change against HEAD?

[Wei James (wistoch)]

Created attachment 156858 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156858 [details]
Patch

Clearing flags on attachment: 156858

Committed r124851: <http://trac.webkit.org/changeset/124851>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.