On Mon, 22 Jul 2024 15:54:18 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> I still think there is a value in providing exhaustive coverage, especially >> since the number of combination is relatively low. The added value, for >> instance, is to guard against regressions introduced by other people. > > @andy-goryachev-oracle > > Could you be more specific which cases (related to this change and lookups) > are currently not being adequately covered? > > I'm of the opinion that the tests are more than sufficient for the changes > involved, I don't think it is the responsibility of this PR to add coverage > in areas unrelated to it. The cases you are requesting IMHO check cascading > rules, which should already be verified elsewhere (and if not, is certainly > not something that should be part of this PR). > > The tests I added include 19(!) new cases (versus the 13 that existed > before), but don't add **any** new coverage (lookups were already covered by > for example `removingThenAddingNodeToDifferentBranchGetsIneritableStyle`) -- > they're only adding assertions specifically for this change. One might even > say I overdid it here, as half of the tests don't even deal with lookups (ie. > all the cases where no "indirect" variant is involved). Making these test > cases exhaustive for something that has 4 variables and 3-4 options each > would result in 80+ cases. > > So adding further tests does not seem to serve any purpose from where I > stand; most of the requested additional cases would not even involve lookups, > or would just confirm existing cascading rules (if an inline style overrides > a property **or** author style, then it would also override property **and** > author style). And none of them would extra coverage... fine ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1503#discussion_r1686847995
