Re: Inheriting annotations into included reftest.list files

[Kartikaya Gupta]

Closing the loop: the changes in bug 1429580 have landed, and so now
doing "skip-if(foo) include bar/reftest.list" behaves as one would
expect, and skips the include entirely if the foo condition is true.

On Wed, Jan 10, 2018 at 4:01 PM, L. David Baron  wrote:
> On Wednesday 2018-01-10 10:49 -0500, Kartikaya Gupta wrote:
>> This will probably come as a surprise to many (as it does to me each
>> time I rediscover it), but if, in a reftest.list file, you do
>> something like this (real example from [1]):
>>
>> skip-if(browserIsRemote) include ogg-video/reftest.list
>>
>> this may not do what you expect. My expectation, at least, is that the
>> entire ogg-video/reftest.list file is skipped if the "browserIsRemote"
>> condition is true.
>>
>> However, what *actually* happens is that the skip-if expected status
>> (which is EXPECTED_DEATH [2]) gets "inherited" down into the included
>> reftest.list, and if there's a higher-valued [3] annotation on a
>> reftest inside that included list, then that will "win". So in
>> practice, this means that any reftest inside ogg-video/reftest.list
>> that has a fuzzy() annotation, or a fuzzy-if(x) annotation where x is
>> true, will still run.
>>
>> This seems like a very unexpected result, and looking through some of
>> the cases where this happens it's not at all clear to me if this was
>> intentional, or if these tests are just running accidentally because
>> nobody realized this would happen.
>>
>> I'm happy to make changes to the reftest manifest parser to remove
>> this footgun (most likely by disallowing annotations on include
>> statements) but we would need to go through each instance of this in
>> the reftest.list files and fix things up so that the tests that are
>> running are in line with the expectations of whoever added/owns the
>> tests.
>>
>> I wanted to open this up for discussion in case people have any
>> thoughts on it before I move forward and try to clean this up.
>
> I think some of the mistake here is related to how fuzzy was
> implemented:
>
> For a start, fuzzy shouldn't have outranked skip in the list of
> statuses.
>
> But really, fuzzy is sometimes used in different ways:  it's
> sometimes used as a variation of the status (expected to pass,
> expected to fail, etc.) but also sometimes used a variation of the
> expected result (==, !=).  It's sort of a mess, since there are
> different understandings of what it should mean that make more or
> less sense for different uses.
>
> But I'd be fine just getting rid of annotations on 'include'
> directives.  They're confusing, and there was a substantial need for
> them for a short period of time (which was mostly about
> transitionally enabling tests on new platforms), but I think it's
> over.
>
> (If we need to keep skip for include directives, maybe we could at
> least get rid of the others, since if you go with dholbert's
> proposal skip would be processed in an entirely different way from
> the others.)
>
> -David
>
> --
> 턞   L. David Baron http://dbaron.org/   턂
> 턢   Mozilla  https://www.mozilla.org/   턂
>  Before I built a wall I'd ask to know
>  What I was walling in or walling out,
>  And to whom I was like to give offense.
>- Robert Frost, Mending Wall (1914)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Inheriting annotations into included reftest.list files

[L. David Baron]

On Wednesday 2018-01-10 10:49 -0500, Kartikaya Gupta wrote:
> This will probably come as a surprise to many (as it does to me each
> time I rediscover it), but if, in a reftest.list file, you do
> something like this (real example from [1]):
> 
> skip-if(browserIsRemote) include ogg-video/reftest.list
> 
> this may not do what you expect. My expectation, at least, is that the
> entire ogg-video/reftest.list file is skipped if the "browserIsRemote"
> condition is true.
> 
> However, what *actually* happens is that the skip-if expected status
> (which is EXPECTED_DEATH [2]) gets "inherited" down into the included
> reftest.list, and if there's a higher-valued [3] annotation on a
> reftest inside that included list, then that will "win". So in
> practice, this means that any reftest inside ogg-video/reftest.list
> that has a fuzzy() annotation, or a fuzzy-if(x) annotation where x is
> true, will still run.
> 
> This seems like a very unexpected result, and looking through some of
> the cases where this happens it's not at all clear to me if this was
> intentional, or if these tests are just running accidentally because
> nobody realized this would happen.
> 
> I'm happy to make changes to the reftest manifest parser to remove
> this footgun (most likely by disallowing annotations on include
> statements) but we would need to go through each instance of this in
> the reftest.list files and fix things up so that the tests that are
> running are in line with the expectations of whoever added/owns the
> tests.
> 
> I wanted to open this up for discussion in case people have any
> thoughts on it before I move forward and try to clean this up.

I think some of the mistake here is related to how fuzzy was
implemented:

For a start, fuzzy shouldn't have outranked skip in the list of
statuses.

But really, fuzzy is sometimes used in different ways:  it's
sometimes used as a variation of the status (expected to pass,
expected to fail, etc.) but also sometimes used a variation of the
expected result (==, !=).  It's sort of a mess, since there are
different understandings of what it should mean that make more or
less sense for different uses.

But I'd be fine just getting rid of annotations on 'include'
directives.  They're confusing, and there was a substantial need for
them for a short period of time (which was mostly about
transitionally enabling tests on new platforms), but I think it's
over.

(If we need to keep skip for include directives, maybe we could at
least get rid of the others, since if you go with dholbert's
proposal skip would be processed in an entirely different way from
the others.)

-David

-- 
턞   L. David Baron http://dbaron.org/   턂
턢   Mozilla  https://www.mozilla.org/   턂
 Before I built a wall I'd ask to know
 What I was walling in or walling out,
 And to whom I was like to give offense.
   - Robert Frost, Mending Wall (1914)


signature.asc
Description: PGP signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Inheriting annotations into included reftest.list files

[Kartikaya Gupta]

On Wed, Jan 10, 2018 at 3:40 PM, Daniel Holbert  wrote:
> I'd lean slightly towards allowing the syntax and making it actually skip
> the include expression.  This construct seems valuable to have in our
> toolbox (to be used only sparingly, e.g. for cases of platform-specific
> features).

Yeah I'm inclined to agree. I looked over the places where we are
currently doing this (the 11 in layout/reftests that you found, plus 3
more in image/test/reftest/reftest.list) and in all cases it seems
like the intent was to just skip the include entirely. So I'm planning
to change the semantics to do that, and disallow other annotations
like "fuzzy include foo/reftest.list" or "fails include
foo/reftest.list" - we don't have any of those currently and I think
disallowing them is the way to go.

I've filed bug 1429580 for this issue.

Cheers,
kats
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Inheriting annotations into included reftest.list files

[Daniel Holbert]

Agreed that this is footgunny & unexpected!

I'd lean slightly towards allowing the syntax and making it actually skip
the include expression.  This construct seems valuable to have in our
toolbox (to be used only sparingly, e.g. for cases of platform-specific
features).

(Based on a quick grep[1], it looks like we only do this 11 times in
layout/reftests right now, which is about what I'd hope/expect.)

[1] grep -r "skip.*include" layout/reftests | wc -l

On Wed, Jan 10, 2018 at 11:30 AM, Kartikaya Gupta 
wrote:

> Another option would be to keep allowing this syntax of "skip-if(x)
> include some/reftest.list" but actually make it skip the entire
> include if the condition "x" is true.
>
> On Wed, Jan 10, 2018 at 10:49 AM, Kartikaya Gupta 
> wrote:
> > This will probably come as a surprise to many (as it does to me each
> > time I rediscover it), but if, in a reftest.list file, you do
> > something like this (real example from [1]):
> >
> > skip-if(browserIsRemote) include ogg-video/reftest.list
> >
> > this may not do what you expect. My expectation, at least, is that the
> > entire ogg-video/reftest.list file is skipped if the "browserIsRemote"
> > condition is true.
> >
> > However, what *actually* happens is that the skip-if expected status
> > (which is EXPECTED_DEATH [2]) gets "inherited" down into the included
> > reftest.list, and if there's a higher-valued [3] annotation on a
> > reftest inside that included list, then that will "win". So in
> > practice, this means that any reftest inside ogg-video/reftest.list
> > that has a fuzzy() annotation, or a fuzzy-if(x) annotation where x is
> > true, will still run.
> >
> > This seems like a very unexpected result, and looking through some of
> > the cases where this happens it's not at all clear to me if this was
> > intentional, or if these tests are just running accidentally because
> > nobody realized this would happen.
> >
> > I'm happy to make changes to the reftest manifest parser to remove
> > this footgun (most likely by disallowing annotations on include
> > statements) but we would need to go through each instance of this in
> > the reftest.list files and fix things up so that the tests that are
> > running are in line with the expectations of whoever added/owns the
> > tests.
> >
> > I wanted to open this up for discussion in case people have any
> > thoughts on it before I move forward and try to clean this up.
> >
> > [1] https://searchfox.org/mozilla-central/rev/
> 03877052c151a8f062eea177f684a2743cd7b1d5/layout/reftests/reftest.list#275
> > [2] https://searchfox.org/mozilla-central/rev/
> 03877052c151a8f062eea177f684a2743cd7b1d5/layout/tools/
> reftest/manifest.jsm#228
> > [3] https://searchfox.org/mozilla-central/rev/
> 03877052c151a8f062eea177f684a2743cd7b1d5/layout/tools/
> reftest/globals.jsm#42
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Inheriting annotations into included reftest.list files

[Kartikaya Gupta]

Another option would be to keep allowing this syntax of "skip-if(x)
include some/reftest.list" but actually make it skip the entire
include if the condition "x" is true.

On Wed, Jan 10, 2018 at 10:49 AM, Kartikaya Gupta  wrote:
> This will probably come as a surprise to many (as it does to me each
> time I rediscover it), but if, in a reftest.list file, you do
> something like this (real example from [1]):
>
> skip-if(browserIsRemote) include ogg-video/reftest.list
>
> this may not do what you expect. My expectation, at least, is that the
> entire ogg-video/reftest.list file is skipped if the "browserIsRemote"
> condition is true.
>
> However, what *actually* happens is that the skip-if expected status
> (which is EXPECTED_DEATH [2]) gets "inherited" down into the included
> reftest.list, and if there's a higher-valued [3] annotation on a
> reftest inside that included list, then that will "win". So in
> practice, this means that any reftest inside ogg-video/reftest.list
> that has a fuzzy() annotation, or a fuzzy-if(x) annotation where x is
> true, will still run.
>
> This seems like a very unexpected result, and looking through some of
> the cases where this happens it's not at all clear to me if this was
> intentional, or if these tests are just running accidentally because
> nobody realized this would happen.
>
> I'm happy to make changes to the reftest manifest parser to remove
> this footgun (most likely by disallowing annotations on include
> statements) but we would need to go through each instance of this in
> the reftest.list files and fix things up so that the tests that are
> running are in line with the expectations of whoever added/owns the
> tests.
>
> I wanted to open this up for discussion in case people have any
> thoughts on it before I move forward and try to clean this up.
>
> [1] 
> https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/layout/reftests/reftest.list#275
> [2] 
> https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/layout/tools/reftest/manifest.jsm#228
> [3] 
> https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/layout/tools/reftest/globals.jsm#42
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Inheriting annotations into included reftest.list files

[Kartikaya Gupta]

This will probably come as a surprise to many (as it does to me each
time I rediscover it), but if, in a reftest.list file, you do
something like this (real example from [1]):

skip-if(browserIsRemote) include ogg-video/reftest.list

this may not do what you expect. My expectation, at least, is that the
entire ogg-video/reftest.list file is skipped if the "browserIsRemote"
condition is true.

However, what *actually* happens is that the skip-if expected status
(which is EXPECTED_DEATH [2]) gets "inherited" down into the included
reftest.list, and if there's a higher-valued [3] annotation on a
reftest inside that included list, then that will "win". So in
practice, this means that any reftest inside ogg-video/reftest.list
that has a fuzzy() annotation, or a fuzzy-if(x) annotation where x is
true, will still run.

This seems like a very unexpected result, and looking through some of
the cases where this happens it's not at all clear to me if this was
intentional, or if these tests are just running accidentally because
nobody realized this would happen.

I'm happy to make changes to the reftest manifest parser to remove
this footgun (most likely by disallowing annotations on include
statements) but we would need to go through each instance of this in
the reftest.list files and fix things up so that the tests that are
running are in line with the expectations of whoever added/owns the
tests.

I wanted to open this up for discussion in case people have any
thoughts on it before I move forward and try to clean this up.

[1] 
https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/layout/reftests/reftest.list#275
[2] 
https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/layout/tools/reftest/manifest.jsm#228
[3] 
https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/layout/tools/reftest/globals.jsm#42
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform