[webkit-dev] Please don't land patches without rebaselining tests for at least one platform

[Ryosuke Niwa]

On Thu, Mar 21, 2013 at 4:50 PM, Glenn Adams <glenn at skynav.com> wrote:

> On Thu, Mar 21, 2013 at 5:40 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
>
>> On Thu, Mar 21, 2013 at 4:36 PM, Glenn Adams <glenn at skynav.com> wrote:
>>
>>>
>>> On Thu, Mar 21, 2013 at 5:10 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
>>>
>>>> On Thu, Mar 21, 2013 at 4:02 PM, Glenn Adams <glenn at skynav.com> wrote:
>>>>
>>>>> On Thu, Mar 21, 2013 at 12:49 PM, Ryosuke Niwa <rniwa at webkit.org>wrote:
>>>>>
>>>>>> Lately, I've encountering changesets that only add lines to
>>>>>> TestExpectations and then never baseline tests for any platform.
>>>>>>
>>>>>
>>>>> This (never rebaseline tests for any platform in that changeset) may
>>>>> not be possible depending on circumstances. For example, I do my dev work
>>>>> on MBP Retina, which produces different baselines than the platforms used
>>>>> for mac test bots. As a result, I sometimes have no choice but to land a
>>>>> changeset without a new baseline and then use garden-o-matic after-the-fact
>>>>> to land a new baseline.
>>>>>
>>>>
>>>> Then how are you verifying that your patch is correct? How are
>>>> reviewers supposed to review such a patch?
>>>>
>>>> Uploading a rendering engine patch without first verifying that tests
>>>> are still passing and new tests are generating results as expected sounds
>>>> like a bad idea to me.
>>>>
>>>
>>> Of course I am verifying that tests are still passing and that new tests
>>> are generating results as expected. I do this by running NRWT without the
>>> patch, running it with the patch, then comparing results. This generally
>>> allows me to see new failures, which I manually review to determine the
>>> nature of the difference. If the difference is simply minor pixel
>>> positioning deltas (in text or image output), then I operate on the
>>> tentative hypothesis that a rebaseline is needed for the given test, unless
>>> I happen to be making a change that could be attributed to cause the delta.
>>>
>>
>> That sounds like a dangerous assumption to make. Due to the DPI
>> differences, things like anti-aliasing will behave quite differently on
>> Retina MBP.
>>
>
> A hypothesis is not an assumption. It needs to be tested further. Be sure
> I am not making any unwarranted assumption.
>

I don't think operating based on a hypothesis is a good idea either.


> In general, I don't recommend people running and relying on layout tests
>> on Retina MBP especially if you work on the rendering engine at this time.
>>
>
> That's my platform, so I have to manage with it.
>

I do have a Retina MBP too but I don't use it to work on the rendering
engine precisely because of this issue.  It's expected that every
contributor has access to a machine where he/she can run layout tests.
 Retina MBP is not such a machine.

 As I've announced on another thread, EWS now uploads actual results on
>>>> Bugzilla so this shouldn't be an issue anymore.
>>>>
>>>
>>> But EWS is only testing on a couple of platform/ports yes? Presumably
>>> this will still impact qt/gtk/efl and perhaps others where EWS is just
>>> building and not testing?
>>>
>>
>> That's fine. I'm asking that you include rebaselines for at least one
>> platform in the case that wasn't clear.
>>
>
> I will if possible, but I'm just saying it may not be possible on the
> initial landing in all cases. If it isn't then I expect to have to add
> rebaseline expectations and then take responsibility for following up on
> them ASAP.
>

What are cases where this is not possible? We need to fix that.

- R. Niwa
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20130321/77b244f6/attachment.html>