Re: [PATCH] Don't mention XFAIL_TESTS anymore
Uli Schlachter <[email protected]>
| Newsgroups | gmane.comp.lib.cairo |
|---|---|
| Message-ID | <[email protected]> |
On 16.09.2013 06:51, Bryce W. Harrington wrote:
> On Sun, Sep 15, 2013 at 08:29:20PM +0100, Chris Wilson wrote:
>> On Sun, Sep 15, 2013 at 02:24:12PM +0200, Uli Schlachter wrote:
>>> The variable XFAIL_TESTS is not used anymore since commit e90073f7ddc.
>>>
>>> Instead, we now have special reference images that show the wrong output and the
>>> test suite fails a test if it does not match the expected, wrong output.
>>
>> It's a reasonable step to bring the text up-to-date. What I feel is
>> lacking is information on which tests are known to be problematic and
>> why - whether or not it is truly fixable. The issue with .xfail is that
>> we lose a central location for such a reference. I guess the README is a
>> better place to explain the XFAIL tests, although it would be best to
>> put detailed explanations in the test themselves.
>
> I agree; I was surprised to discover this info embedded in the
> Makefile.am.
>
> +1 to moving the documentation to the tests themselves, that's probably
> the first place one would look, and it would make it easier to keep the
> notes up to date along with the test. If the explanations include some
> common tag, then we could search for tests with xfail images but no
> explanation, and deal with them.
Done, see the attached patches. This now seemed big enough to warrant splitting
into two patches.
> Also, I was curious if either of you knew if those documented rationales
> are still valid with the current codebase?
Some of the rationales were obviously wrong (see the commit message). For the
rest: Dunno, cairo's CVS times were long before I ever looked at it.
Cheers,
Uli
--
<alanc> I think someone had a Xprint version of glxgears at one point,
but benchmarking how many GL pages you can print per second
was deemed too silly to merge
--
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo
0001-Remove-XFAIL_TESTS-from-Makefile.am.patch
(text/x-patch, 12.2 KB)
From 5af13b6c73ce365d6ccc8fc8b4062f75395f9b6f Mon Sep 17 00:00:00 2001 From: Uli Schlachter <[email protected]> Date: Sun, 29 Sep 2013 13:12:55 +0200 Subject: [PATCH 1/2] Remove XFAIL_TESTS from Makefile.am The variable XFAIL_TESTS is not used anymore since commit e90073f7ddc. The description for the known failures are moved into the respective tests as comments. The following descriptions were dropped: - surface-pattern-big: Didn't really explain the failure - big-line: Test isn't failing any more - self-intersecting: Only XFAIL on quartz, but description doesn't match this The following tests don't have a xfail reference image and seem to fail just because of not having a reference image at all (I kept their description for now): big-trap, long-lines, self-copy-overlap Signed-off-by: Uli Schlachter <[email protected]> --- test/Makefile.am | 73 ------------------------------------------ test/alpha-similar.c | 4 +++ test/big-trap.c | 1 + test/degenerate-dash.c | 5 +++ test/degenerate-path.c | 4 +++ test/device-offset-scale.c | 4 +++ test/extend-pad.c | 4 +++ test/fallback-resolution.c | 10 ++++++ test/in-fill-empty-trapezoid.c | 5 +++ test/long-lines.c | 1 + test/scale-offset-image.c | 1 + test/scale-offset-similar.c | 1 + test/self-copy-overlap.c | 5 +++ 13 files changed, 45 insertions(+), 73 deletions(-) diff --git a/test/Makefile.am b/test/Makefile.am index a9495dc..547e5d2 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -142,79 +142,6 @@ surface-source.c \ testtable.js \ reference -# Any test for which the code committed to CVS is expected to fail -# should be listed here. -# -# This way, we can avoid being bothered by reports of bugs we are -# aware of, but users can still report when tests start behaving in -# unexpected ways on their system. -# -# Of course, before any "release" of cairo we should eliminate -# everything from this list by fixing the bugs. (We don't necessarily -# have to be that strict for "snapshots" though.) -# -# Analysis of XFAIL errors: -# alpha-similar - discrepancy between backends in applying color -# components of a pure alpha surface -# big-line - range overflow of fixed-point -# big-trap - range overflow of fixed-point -# degenerate-dash - needs path editing in PS to convert degenerate -# end-caps into the shapes as expected by cairo -# (Or maybe PS is the correct behaviour?) -# degenerate-path - undefined behaviour in PS, needs path editing to -# convert degenerate segments into circles/rectangles -# as expected by cairo -# device-offset-scale - complication of pre-multiplying device_offset -# into the pattern_matrix and then requiring further -# manipulation for SVG -# extend-pad - lacks implementation in pixman and consequently used -# as an excuse for lack of support in other backends -# fallback-resolution - The essential problem here is that the recording-surface -# has recorded a sequence of operations with one device -# transformation, and we attempt to replay it with -# another (basically a scale-factor for the falback -# resolution). Carl begun to look at this with his -# chain-of-bugs, but the can of worms is much bigger. -# It appears to be a design flaw in the recording-surface -# that may spread further... -# My solution would be to lock Behad and Adrian in a -# room, with Carl as a moderator and not let them out -# until they have come up with an interface and -# semantics that actually work. :-) -# in-fill-empty-trapezoid The cairo_in_fill () function can sometimes -# produce false positives when the tessellator -# produces empty trapezoids and the query -# point lands exactly on a trapezoid edge. -# long-lines - range overflow of fixed-point -# scale-offset-image - loss of precision converting a cairo matrix to -# scale-offset-similar pixman's fixed point format. -# self-copy-overlap - vector surfaces take snapshot of patterns in contrast -# to the raster backends which don't. One solution -# would be to clone overlapping areas of dst/source, so -# patterns were effectively snapshotted across all -# backends. -# self-intersecting - incremental trapezoidation of strokes can generate -# overlapping traps. Needs self-intersection analysis -# on cairo_traps_t after strokes. -# Test case should also be expanded to hit special-case -# tessellators as well. -# surface-pattern-big...- Strange, unexplained results for SVG/PS. -XFAIL_TESTS = \ -alpha-similar$(EXEEXT) \ -big-line$(EXEEXT) \ -big-trap$(EXEEXT) \ -degenerate-dash$(EXEEXT) \ -degenerate-path$(EXEEXT) \ -device-offset-scale$(EXEEXT) \ -extend-pad$(EXEEXT) \ -fallback-resolution$(EXEEXT) \ -in-fill-empty-trapezoid$(EXEEXT) \ -long-lines$(EXEEXT) \ -self-copy-overlap$(EXEEXT) \ -self-intersecting$(EXEEXT) \ -surface-pattern-big-scale-down$(EXEEXT) \ -$(NULL) - # Any test that doesn't generate a log file goes here NOLOG_TESTS = \ fallback-resolution \ diff --git a/test/alpha-similar.c b/test/alpha-similar.c index a9411d1..86c6ae3 100644 --- a/test/alpha-similar.c +++ b/test/alpha-similar.c @@ -60,6 +60,10 @@ draw (cairo_t *cr, int width, int height) return CAIRO_TEST_SUCCESS; } +/* + * XFAIL: discrepancy between backends in applying color components of a pure + * alpha surface + */ CAIRO_TEST (alpha_similar, "Tests creation of similar alpha surfaces" "\nApplication of a pure-alpha similar source is inconsistent across backends.", diff --git a/test/big-trap.c b/test/big-trap.c index 507e0e5..5e11237 100644 --- a/test/big-trap.c +++ b/test/big-trap.c @@ -82,6 +82,7 @@ draw (cairo_t *cr, int width, int height) return CAIRO_TEST_SUCCESS; } +/* XFAIL: range overflow of fixed-point */ CAIRO_TEST (big_trap, "Test oversize trapezoid with a clip region" "\nTest needs to be adjusted to trigger the original bug", diff --git a/test/degenerate-dash.c b/test/degenerate-dash.c index c13792f..12cd0c7 100644 --- a/test/degenerate-dash.c +++ b/test/degenerate-dash.c @@ -77,6 +77,11 @@ draw (cairo_t *cr, int width, int height) return CAIRO_TEST_SUCCESS; } +/* + * XFAIL: needs path editing in PS to convert degenerate + * end-caps into the shapes as expected by cairo (Or maybe PS is the correct + * behaviour?) + */ CAIRO_TEST (degenerate_dash, "Tests the behaviour of dashed segments that end on a off-on transition", "dash, degenerate", /* keywords */ diff --git a/test/degenerate-path.c b/test/degenerate-path.c index c690730..db53cbb 100644 --- a/test/degenerate-path.c +++ b/test/degenerate-path.c @@ -108,6 +108,10 @@ draw (cairo_t *cr, int width, int height) return CAIRO_TEST_SUCCESS; } +/* + * XFAIL: undefined behaviour in PS, needs path editing to convert degenerate + * segments into circles/rectangles as expected by cairo + */ CAIRO_TEST (degenerate_path, "Tests the behaviour of degenerate paths with different cap types", "degenerate", /* keywords */ diff --git a/test/device-offset-scale.c b/test/device-offset-scale.c index 3df3d38..33784fa 100644 --- a/test/device-offset-scale.c +++ b/test/device-offset-scale.c @@ -65,6 +65,10 @@ draw (cairo_t *cr, int width, int height) return CAIRO_TEST_SUCCESS; } +/* + * XFAIL: complication of pre-multiplying device_offset into the pattern_matrix + * and then requiring further manipulation for SVG + */ CAIRO_TEST (device_offset_scale, "Test that the device-offset transform is transformed by the ctm.", "device-offset", /* keywords */ diff --git a/test/extend-pad.c b/test/extend-pad.c index 15a2079..16b171a 100644 --- a/test/extend-pad.c +++ b/test/extend-pad.c @@ -73,6 +73,10 @@ draw (cairo_t *cr, int width, int height) return CAIRO_TEST_SUCCESS; } +/* + * XFAIL: lacks implementation in pixman and consequently used as an excuse for + * lack of support in other backends + */ CAIRO_TEST (extend_pad, "Test CAIRO_EXTEND_PAD for surface patterns", "extend", /* keywords */ diff --git a/test/fallback-resolution.c b/test/fallback-resolution.c index df53e62..84ef082 100644 --- a/test/fallback-resolution.c +++ b/test/fallback-resolution.c @@ -504,6 +504,16 @@ preamble (cairo_test_context_t *ctx) return ret; } +/* + * XFAIL: The essential problem here is that the recording-surface has recorded + * a sequence of operations with one device transformation, and we attempt to + * replay it with another (basically a scale-factor for the falback resolution). + * Carl begun to look at this with his chain-of-bugs, but the can of worms is + * much bigger. It appears to be a design flaw in the recording-surface that + * may spread further... My solution would be to lock Behad and Adrian in a + * room, with Carl as a moderator and not let them out until they have come up + * with an interface and semantics that actually work. :-) + */ CAIRO_TEST (fallback_resolution, "Check handling of fallback resolutions", "fallback", /* keywords */ diff --git a/test/in-fill-empty-trapezoid.c b/test/in-fill-empty-trapezoid.c index 2b345cb..0797399 100644 --- a/test/in-fill-empty-trapezoid.c +++ b/test/in-fill-empty-trapezoid.c @@ -96,6 +96,11 @@ preamble (cairo_test_context_t *ctx) return ret; } +/* + * XFAIL: The cairo_in_fill () function can sometimes produce false positives + * when the tessellator produces empty trapezoids and the query point lands + * exactly on a trapezoid edge. + */ CAIRO_TEST (in_fill_empty_trapezoid, "Test that the tessellator isn't producing obviously empty trapezoids", "in, trap", /* keywords */ diff --git a/test/long-lines.c b/test/long-lines.c index 79fc98f..69b64e9 100644 --- a/test/long-lines.c +++ b/test/long-lines.c @@ -74,6 +74,7 @@ draw (cairo_t *cr, int width, int height) return CAIRO_TEST_SUCCESS; } +/* XFAIL: range overflow of fixed-point */ CAIRO_TEST (long_lines, "Test long lines" "\nLong lines are not drawn due to the limitations of the internal 16.16 fixed-point coordinates", diff --git a/test/scale-offset-image.c b/test/scale-offset-image.c index a6e575a..7243a06 100644 --- a/test/scale-offset-image.c +++ b/test/scale-offset-image.c @@ -133,6 +133,7 @@ draw (cairo_t *cr, int width, int height) return CAIRO_TEST_SUCCESS; } +/* XFAIL: loss of precision converting a cairo matrix to */ CAIRO_TEST (scale_offset_image, "Tests drawing surfaces under various scales and transforms", "surface, scale-offset", /* keywords */ diff --git a/test/scale-offset-similar.c b/test/scale-offset-similar.c index e9e6a2b..8955cbe 100644 --- a/test/scale-offset-similar.c +++ b/test/scale-offset-similar.c @@ -134,6 +134,7 @@ draw (cairo_t *cr, int width, int height) return CAIRO_TEST_SUCCESS; } +/* XFAIL: pixman's fixed point format. */ CAIRO_TEST (scale_offset_similar, "Tests drawing surfaces under various scales and transforms", "surface, scale-offset", /* keywords */ diff --git a/test/self-copy-overlap.c b/test/self-copy-overlap.c index 2f49ab0..6e3b39c 100644 --- a/test/self-copy-overlap.c +++ b/test/self-copy-overlap.c @@ -40,6 +40,11 @@ draw (cairo_t *cr, int width, int height) return CAIRO_TEST_SUCCESS; } +/* + * XFAIL: vector surfaces take snapshot of patterns in contrast to the raster + * backends which don't. One solution would be to clone overlapping areas of + * dst/source, so patterns were effectively snapshotted across all backends. + */ CAIRO_TEST (self_copy_overlap, "Tests painting to itself using itself as the source" "\nBackends treat this case inconsistently---vector backends are creating snapshots.", -- 1.8.4.rc3
0002-README-Don-t-mention-XFAIL_TESTS-anymore.patch
(text/x-patch, 2.5 KB)
From 388bac5f524570698cb354995e8ee89611f3f15f Mon Sep 17 00:00:00 2001 From: Uli Schlachter <[email protected]> Date: Sun, 15 Sep 2013 14:05:42 +0200 Subject: [PATCH 2/2] README: Don't mention XFAIL_TESTS anymore The variable XFAIL_TESTS is not used anymore since commit e90073f7ddc. Instead, we now have special reference images that show the wrong output and the test suite fails a test if it does not match the expected, wrong output. Signed-off-by: Uli Schlachter <[email protected]> --- test/README | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/test/README b/test/README index 3f19a5c..e8e7189 100644 --- a/test/README +++ b/test/README @@ -192,16 +192,16 @@ A new test case should be added by imitating the style of an existing test. This means adding the following files: new-bug.c - new-bug-ref.png + reference/new-bug.ref.png + reference/new-bug.xfail.png Where new-bug.c is a minimal program to demonstrate the bug, following -the style of existing tests. The new-bug-ref.png image should contain -the desired result of new-bug.c if the bug were fixed. +the style of existing tests. The new-bug.ref.png image should contain +the desired result of new-bug.c if the bug were fixed while +new-bug.xfail.png contains the current results of the test. -Makefile.am should be edited, adding new-bug.c to both the TESTS and -XFAIL_TESTS lists and new-bug-ref.png to EXTRA_DIST. Add new-bug to -.gitignore, and last but not least, don't forget to "git add" the new -files. +Makefile.sources should be edited by adding new-bug.c to test_sources. +And last but not least, don't forget to "git add" the new files. When a new feature is added --------------------------- @@ -209,7 +209,7 @@ It's important for the regression suite to keep pace with development of the library. So a new test should be added for each new feature. The work involved is similar the work described above for new bugs. The only distinction is that the test is expected to pass so it -should not be added to the XFAIL_TESTS list. +should not need a new-bug.xfail.png file. While working on a test ----------------------- @@ -223,8 +223,7 @@ When a bug is fixed ------------------- The fix should be verified by running the test suite which should result in an "unexpected pass" for the test of interest. Rejoice as -appropriate, then remove the relevant file name from the XFAIL_TESTS -variable in Makefile.am. +appropriate, then remove the relevant xfail.png file from git. Before releasing ---------------- -- 1.8.4.rc3