1817173 - Partial Test For `twoFloats operator+(const Point& aPoint)`

Status: ASSIGNED

(Core :: Web Painting, enhancement)

Description

[Carolin Brandt]

Attachment: test.html

---

This report is part of a research collaboration between Mozilla and the TU Delft.
If you want to help us understand whether this report is helpful,
please answer a few questions before you start addressing the report.

If you would rather talk to us or show your process on screen, you can schedule a call (write Carolin at [e-mail]) or upload a screen recording.

---

We created a test case that executes the not-yet-tested code block at https://searchfox.org/mozilla-central/source/layout/painting/nsCSSRenderingBorders.cpp#2590.
However, the test is missing a functional check (is(..) or ok(..)) to check that the behavior of the code block is correct.

Please complete the test and add it to the test suite, if you think it is worth to do so.

In the attachments we provide the generated test (test.html).
It reaches the targeted code block through the stacktrace in stacktrace.txt.
We also provide some additional generated tests that target the same location (alternative_test_N.html).

Comment 1

[Carolin Brandt]

Attachment: stacktrace.txt

Comment 2

[Carolin Brandt]

Attachment: alternative_test_1.html

Comment 3

[Carolin Brandt]

Future changes could make the searchfox link invalid.
Here is a permalink to the code targeted by our test case: https://searchfox.org/mozilla-central/rev/00ea1649b59d5f427979e2d6ba42be96f62d6e82/layout/painting/nsCSSRenderingBorders.cpp#2590

Comment 4

[Marco Castelluccio [:marco]]

:arai, could you suggest someone who could help with this? This is a research experiment and it'd be great to get someone to look into this. If you think the target/location is unsuitable for writing a test, let us know (and why). Thanks in advance!

Comment 5

[Tooru Fujisawa [:arai]]

I'll take this tomorrow.

Comment 6

[Tooru Fujisawa [:arai]]

Some part of nsCSSRenderingBorders.cpp is not shown as covered in searchfox because of the following reason:
searchfox uses linux's and windows's coverage data, and on those platforms (and I think on most platforms), the border rendering is mainly done by WebRender,
that replaced the border rendering which had been done by the logic in nsCSSRenderingBorders.cpp,
and the code in nsCSSRenderingBorders.cpp is used only as fallback case, where WebRender cannot handle.

the code had been covered by existing tests, but no longer covered because there's not much testcases which hits the fallback path.

the test case in comment #0 hits the fallback path because of background-clip: text style, which webrender cannot handle.

Possible solution here is either:

• (a) add reftest variant which forces always using the fallback path, and run existing related tests with it
• (b) duplicate some related tests and modify them to use fallback path

and I'd like to go with (a), to avoid duplication.
So the most of the work done here would be to modify the test harness to support the new behavior, instead of touching/creating each testcase.
and I feel it's slightly different than the main target of the research.
(I might go with (b) if (a) doesn't work tho)

can I have your opinion?

Comment 7

[Tooru Fujisawa [:arai]]

actually, the code path is taken on "reftest snapshot", which uses fallback path during taking the screen snapshot.

https://treeherder.mozilla.org/jobs?repo=try&revision=f05076abd4221ab68bd6447836a80ad5f88fe324&selectedTaskRun=coBH7qu9TwOMW8r0juYb6w.0

So, what we need to do here would be to integrate the "reftest snapshot" execution into the code coverage result.

Comment 8

[Tooru Fujisawa [:arai]]

:decoder, what's the set of reftest code-coverage data source?
if it doesn't contain "reftest-snapshot" variant, can we add it to the data source?

https://searchfox.org/mozilla-central/rev/af78418c4b5f2c8721d1a06486cf4cf0b33e1e8d/taskcluster/ci/test/reftest.yml#236-237

reftest-snapshot:
    description: "Reftest snapshot"

https://searchfox.org/mozilla-central/rev/af78418c4b5f2c8721d1a06486cf4cf0b33e1e8d/testing/mozharness/configs/unittests/linux_unittest.py#221-227

"reftest-snapshot": {
    "options": [
        "--suite=reftest",
        "--setpref=reftest.use-draw-snapshot=true",
        "--topsrcdir=tests/reftest/tests",
    ],
    "tests": ["tests/reftest/tests/layout/reftests/reftest.list"],

Comment 9

[Carolin Brandt]

(In reply to Tooru Fujisawa [:arai] from comment #6)

Some part of nsCSSRenderingBorders.cpp is not shown as covered in searchfox because of the following reason:
searchfox uses linux's and windows's coverage data, and on those platforms (and I think on most platforms), the border rendering is mainly done by WebRender,
that replaced the border rendering which had been done by the logic in nsCSSRenderingBorders.cpp,
and the code in nsCSSRenderingBorders.cpp is used only as fallback case, where WebRender cannot handle.

the code had been covered by existing tests, but no longer covered because there's not much testcases which hits the fallback path.

the test case in comment #0 hits the fallback path because of background-clip: text style, which webrender cannot handle.

Possible solution here is either:

• (a) add reftest variant which forces always using the fallback path, and run existing related tests with it
• (b) duplicate some related tests and modify them to use fallback path

and I'd like to go with (a), to avoid duplication.
So the most of the work done here would be to modify the test harness to support the new behavior, instead of touching/creating each testcase.
and I feel it's slightly different than the main target of the research.
(I might go with (b) if (a) doesn't work tho)

can I have your opinion?

Hey Tooru :)

Definetely go with what you find the best solution here. So (a) or ammending the code coverage data source to include the reftest-snapshot.
It is different from what the original report hints at, but our main goal is to see if we can help developers with these tests, and your answers here help us towards that 👍🏼

Curious to hear what :decoder says for the coverage data.

Thank you!

Comment 10

[Christian Holler (:decoder)]

Forwarding NI to marco, he might know this better.

Comment 11

[Marco Castelluccio [:marco]]

reftest-snapshot is only run on linux1804-64-qr/debug: https://searchfox.org/mozilla-central/rev/af78418c4b5f2c8721d1a06486cf4cf0b33e1e8d/taskcluster/ci/test/reftest.yml#240-243. We could run it on ccov too.

Is it worth running all of the reftests with the fallback path, or could only a subset of tests be affected?

Comment 12

[Tooru Fujisawa [:arai]]

thank you :)

we can focus on the subset of tests, where fallback path is certainly taken,
and once the fallback path disappears by extending the WebRender support, we can remove the coverage job.

then, about the subset, what I know is the border radius rendering tests, inside
https://searchfox.org/mozilla-central/source/layout/reftests/border-radius

there will be some more cases I'm not aware of, but we could add that later.