After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 699601 - Add --enable-installed-tests, use it for reftests
Add --enable-installed-tests, use it for reftests
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2013-05-03 16:25 UTC by Colin Walters
Modified: 2013-05-09 17:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001-reftests-Use-pixbuf-property-for-images-to-be-relati.patch (1.49 KB, patch)
2013-05-03 16:26 UTC, Colin Walters
none Details | Review
0001-Add-enable-installed-tests-use-it-for-reftests.patch (3.85 KB, patch)
2013-05-03 16:27 UTC, Colin Walters
none Details | Review
0001-Revert-tests-Add-reftest-for-broken-button-sizing.patch (17.43 KB, patch)
2013-05-03 21:18 UTC, Colin Walters
none Details | Review
0001-Add-enable-installed-tests-use-it-for-reftests.patch (5.08 KB, patch)
2013-05-03 21:18 UTC, Colin Walters
none Details | Review

Description Colin Walters 2013-05-03 16:25:51 UTC
See https://live.gnome.org/GnomeGoals/InstalledTests
---
 configure.ac                 |    6 ++++++
 tests/reftests/Makefile.am   |   39 +++++++++++++++++++++++++++++++++------
 tests/reftests/gtk-reftest.c |    2 +-
 3 files changed, 40 insertions(+), 7 deletions(-)
Comment 1 Colin Walters 2013-05-03 16:26:45 UTC
Created attachment 243215 [details] [review]
0001-reftests-Use-pixbuf-property-for-images-to-be-relati.patch
Comment 2 Colin Walters 2013-05-03 16:27:13 UTC
Created attachment 243216 [details] [review]
0001-Add-enable-installed-tests-use-it-for-reftests.patch
Comment 3 Benjamin Otte (Company) 2013-05-03 16:53:19 UTC
(In reply to comment #1)
> Created an attachment (id=243215) [details] [review]
> 0001-reftests-Use-pixbuf-property-for-images-to-be-relati.patch

I'm using the file property on purpose in that test, because the file property goes through gdk_pixbuf_animation_new_from_file() while the pixbuf property goes through gdk_pixbuf_new_from_file(), and the test is mostly testing brokenness in gdk-pixbuf (well, and making sure the file property works, is a good idea, too).

Can and should we instead maybe set the current working directory to the directory the test is loaded from?
Comment 4 Benjamin Otte (Company) 2013-05-03 17:15:52 UTC
Review of attachment 243216 [details] [review]:

::: tests/reftests/Makefile.am
@@ +36,3 @@
+	button-wrapping.ui \
+	button-wrapping.ref.ui \
+	$(NULL)

I'd rather fix that test than anyone putting anything xfail in Makefiles. (Yes, I committted that test prematurely, sorry)

I'd rather you git revert the patch committing the test than having an xfail.

::: tests/reftests/gtk-reftest.c
@@ +584,3 @@
         basedir = g_getenv ("srcdir");
       else
+        basedir = INSTTESTDIR;

This breaks uninstalled ./gtk-reftest though, doesn't it? And I think that's something I'd like to keep working. Not sure if gtk-reftest can detect if it's installed or not?

If not, it might be beneficial to just install a gtk-reftest-runner script that runs srcdir="/path/to/installed" gtk-reftest or so?
Comment 5 Colin Walters 2013-05-03 17:27:22 UTC
(In reply to comment #3)
> (In reply to comment #1)
> > Created an attachment (id=243215) [details] [review] [details] [review]
> > 0001-reftests-Use-pixbuf-property-for-images-to-be-relati.patch
> 
> I'm using the file property on purpose in that test, because the file property
> goes through gdk_pixbuf_animation_new_from_file() while the pixbuf property
> goes through gdk_pixbuf_new_from_file(), and the test is mostly testing
> brokenness in gdk-pixbuf (well, and making sure the file property works, is a
> good idea, too).

Hmmm...is there that much of a difference?  Don't we have test coverage of those elsewhere?

> Can and should we instead maybe set the current working directory to the
> directory the test is loaded from?

The current InstalledTests specification says that each test is given a clean empty temporary directory that is automatically cleaned up, mainly because for a lot of tests that's very useful.

Now while we *could* change the cwd, the problem with that is that we need to output the .diff.png and such to the test tempdir, because the test doesn't have write access to the directory it's contained in (/usr/libexec/gtk+/installed-tests/reftests is owned by root).
Comment 6 Colin Walters 2013-05-03 17:37:13 UTC
And it'd obviously be nice to say keep around the tmpdir on fail, so any lookaside artifacts like these PNG files could be viewed.  But I can't do that unless the tests write into a well known directory.

Maybe we could add gtk_builder_set_base_directory() or something?
Comment 7 Colin Walters 2013-05-03 21:18:28 UTC
Created attachment 243248 [details] [review]
0001-Revert-tests-Add-reftest-for-broken-button-sizing.patch
Comment 8 Colin Walters 2013-05-03 21:18:53 UTC
Created attachment 243249 [details] [review]
0001-Add-enable-installed-tests-use-it-for-reftests.patch
Comment 9 Benjamin Otte (Company) 2013-05-09 17:14:35 UTC
I've reworked the patches a bit with a bunch of IRC discussions with involved people and pushed them to master. If anything's still borked we can fix it there.