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 <kgu...@mozilla.com> 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 <kgu...@mozilla.com> > 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
