On Mon, 22 Jul 2024 14:50:33 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> I added a few cases where user agent is `null`. I've not added all >> combinations as that would lead to 20-30 extra cases that add little of >> value. If I already verified that INLINE will override the lower priority >> sources individually, then it's safe to say that all possible other >> combinations of those lower priority sources will also get overridden. > > 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... ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1503#discussion_r1686779185
