RFR: 8336097: UserAgent Styles using lookups are promoted to Author level if look-up is defined in Author stylesheet [v2]
John Hendrikx
jhendrikx at openjdk.org
Mon Jul 22 16:01:13 UTC 2024
On Mon, 22 Jul 2024 14:50:33 GMT, Andy Goryachev <angorya at 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
More information about the openjfx-dev
mailing list