[Webkit-unassigned] [Bug 55457] [NRWT] Add support for reftests to new-run-webkit-tests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=55457





--- Comment #7 from Hayato Ito <hayato at chromium.org>  2011-03-09 01:37:40 PST ---
(From update of attachment 85028)
View in context: https://bugs.webkit.org/attachment.cgi?id=85028&action=review

>> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:85
>> +                           ' expected-html file and expected-mismatch.html file'
> 
> nit: s/expected-html/expected.html

Done

>> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:117
>> +                return TestResult(self._filename)
> 
> Should this return None and then the calling code handles None appropriately? That seems cleaner to me at first glance, but I'm not 100% sure how this code is used.

I don't think returning None as a TestResult is good idea. That causes the callers burdens and things more complex.
I agree this 'Dummy' TestResult is not the best way. That's ugly.
Ideally we should skip calling single_test_runner itself if the test is a reftest in rebasing. But that requires yet another non-trivial changes.
So I'd like to leave this code as is until we have a better way in rebasing.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:188
>> +              This arg is optional and shoulbe be used only in reftests until we have a better way to know
> 
> s/shoulbe/should

Done

>> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:200
>> +            # FIXME: Move this actual file writing code somewhere so that this fucntion doesn't have any side effects.
> 
> s/fucntion/function

Done

>> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:269
>> +        max_test_time = max(driver_output1.test_time, driver_output2.test_time)
> 
> This should  probably just be the sum of the test times since we use this information to see how long a test took to run.

Done.  I used 'sum' instead of 'max'.

Let me explain my thought.
Actually I had thought which was better between 'max' and 'sum'.

I have an concern using 'sum'. It might confuse users or flakiness dashboard in the following case:

  - timeout limit: 10s
  - html1 took: 8s (it's okay because 8s < 10s)
  - html2 took: 7s (it's okay because 8s < 10s)
  - sum: 15s

Users might be upset because displayed test time is bigger than '10s', but TIMEOUT didn't happen.
Ideally, we should preserve both test_time values from both html files and display both values. But that is not realistic due to the current layout tests convention.
We should compromise on this.

If you have any ideas, please let me know.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:283
>> +            if driver_output1.image_hash != driver_output2.image_hash:
> 
> This can be an "elif"

Done

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:310
>> +                fn in out_names]
> 
> fn is not a clear variable name. out_filename perhaps?

Done. Thank you. I don't remember why I chose such a name.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:328
>> +                fn in out_names]
> 
> ditto

Done

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:350
>> +    def expected_html_filename(self, filename):
> 
> how about calling this reftest_expected_filename? The fact that it ends in .html is an implementation detail.

Done. Although I don't think using 'html' is so bad because '.html' is only supported in current implementation and it's intuitive.
But I don't have a strong opinion. Either is okay for me. Thank you.

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:354
>> +    def expected_mismatch_html_filename(self, filename):
> 
> How about calling this reftest_expected_mismatch_filename?

Done. Thank you again.

>> Tools/Scripts/webkitpy/layout_tests/port/test.py:53
>> +        self.without_expectation_files = False
> 
> should this just be self.is_reftest?

That makes sense. Done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.