GNOME Bugzilla – Bug 730505
Draft for atspi tests
Last modified: 2014-12-09 14:53:34 UTC
Created attachment 276923 [details] [review] Patch has tests draft implementation Overview: This draft for tests consists of 3 accessibility interface tests, it based on pyatspi2 testing approach. To build those tests autogen should be called with parameter "--with-atspi-tests=yes", tests will be build. To run those tests `make check` should be called, report is genereated in atspi-tests directory.
Review of attachment 276923 [details] [review]: This is a first review, and probably will not find all the issues, but I think that it would be better to do a iterative review. Note that in several cases, I made a review for just one of the my-atk-xxx objects, but the review applies to all of them (for example, the use of G_DEFINE_TYPE macros). In any case, it is good to have a more complete dummy atk implementation here. Some months ago I revamped atk/tests, and it has some examples of how to implement some atk objects. I used it to test the new functionality, and I had some ideas about extending it in order to have a default atk implementation. But probably it is true that it is better to have it here, where it could be properly tested. In any case, you can look at atk/tests for reference if needed. ::: atspi-tests/atk-object-xml-loader.c @@ +1,2 @@ +/* + * Copyright 2008 Codethink Ltd. If this is a derivative work, I think that you also hold some of the copyright. ::: atspi-tests/dummyatk/my-atk-action.c @@ +7,3 @@ + +static GObjectClass *parent_class = NULL; +//implementation of the interface Comment not really needed. Try to avoid c++ style comments. @@ +10,3 @@ +static gboolean my_atk_action_do_action(AtkAction *action, gint i) +{ + MyAtkAction *self = (MyAtkAction*)action; You defined an MY_ATK_ACTION macro on my-atk-action.h to do this kind of castings. @@ +17,3 @@ +static gint my_atk_action_get_n_actions(AtkAction *action) +{ + MyAtkAction *self = (MyAtkAction*)action; ditto. @@ +151,3 @@ + return; + } + self->disposed = TRUE; Why saving if was disposed, and this check is needed? @@ +188,3 @@ + parent_class = g_type_class_peek_parent(klass); +} +GType my_atk_action_get_type(void) Unless something really specific is needed, these days it is not needed to fully implement all the classes get_type. As you probably has realized, this has a lot of boilerplate code, so now there is available several macros (G_DEFINE_TYPE, G_DEFINE_TYPE_WITH_CODE, etc) If you want an example of them used for a dummy atk class, you can take a look to the small tests available on atk, like: https://git.gnome.org/browse/atk/tree/tests/testdocument.c#n51 ::: atspi-tests/dummyatk/my-atk-action.h @@ +1,3 @@ +#ifndef MY_ATK_ACTION_H +#define MY_ATK_ACTION_H +//Object, which implement interface AtkAction(all functions) It would be better to have this kind of documentation on the .c file. Also it would be good too avoid C++ like comments. @@ +8,3 @@ +#include "my-atk-object.h" + +//declarations I don't see this comment really needed, the following is usual glib declarations. @@ +28,3 @@ + + +//for external using ditto @@ +47,3 @@ + }*actions; + gint n; + gint last_performed_action;//this field is changed when perfoms action Although this will never be published as a public library. Could it possible to save all that information on a private structure? ::: atspi-tests/dummyatk/my-atk-component.c @@ +353,3 @@ + + klass->get_extents = my_atk_component_get_extents; + klass->get_position = my_atk_component_get_position; get_position is deprecated. @@ +354,3 @@ + klass->get_extents = my_atk_component_get_extents; + klass->get_position = my_atk_component_get_position; + klass->get_size = my_atk_component_get_size; get_size is deprecated @@ +367,3 @@ + + klass->grab_focus = my_atk_component_grab_focus; + klass->add_focus_handler = my_atk_component_add_focus_handler; add_focus_handler is deprecated. @@ +368,3 @@ + klass->grab_focus = my_atk_component_grab_focus; + klass->add_focus_handler = my_atk_component_add_focus_handler; + klass->remove_focus_handler = my_atk_component_remove_focus_handler; remove_focus_handler is deprecated ::: atspi-tests/dummyatk/my-atk-table.c @@ +39,3 @@ + /* set up overrides here */ + klass-> ref_at = + (AtkObject* (*) (AtkTable *table, gint row, gint column)) NULL; Is all this casting really needed? @@ +41,3 @@ + (AtkObject* (*) (AtkTable *table, gint row, gint column)) NULL; + klass-> get_index_at = + (gint (*) (AtkTable *table, gint row, gint column)) NULL; get_index_at is deprecated. @@ +43,3 @@ + (gint (*) (AtkTable *table, gint row, gint column)) NULL; + klass-> get_column_at_index = + (gint (*) (AtkTable *table, gint index_)) NULL; get_column_at_index is deprecated @@ +45,3 @@ + (gint (*) (AtkTable *table, gint index_)) NULL; + klass-> get_row_at_index = + (gint (*) (AtkTable *table, gint index_)) NULL; get_row_at_index is deprecated ::: atspi-tests/dummyatk/my-atk-text.c @@ +1216,3 @@ + klass->get_text_after_offset = my_atk_text_get_text_after_offset; + klass->get_text_at_offset = my_atk_text_get_text_at_offset; + klass->get_text_before_offset = my_atk_text_get_text_before_offset; get_text_at_offset, get_text_before_offset and get_text_after_offset are deprecated. Now you only have the method get_string_at_offset ::: atspi-tests/test-application.c @@ +26,3 @@ + * This file contains the entry point for all test applications. + * Each test is built as a GModule, and this program loads the + * test module, as well as the AT-SPI module. The test module will See below. The at-spi-atk module is not needed to be loaded, and it is advisable to use it as a library. Having said so, what it is the advantage of using gmodule to execute each individual test? @@ +92,3 @@ +typedef void (*GtkModuleInit) (int *argc, char **argv[]); + +/* AT-SPI is a gtk module that must be loaded and initialized */ s/AT-SPI/AT-SPI Atk bridge. In any case, since gnome 3.6 it is possible to use as a library, avoiding the hassle of using gmodule. Unfourtunately, it is still not html-documented. Here the documentation at code: https://git.gnome.org/browse/at-spi2-atk/tree/atk-adaptor/bridge.c#n971 And here an example of usage, specifically how it was used on gnome-shell: https://git.gnome.org/browse/gnome-shell/tree/src/main.c#n248 I you want to even further help, you could start the documentation for the atk bridge module, based on your experience ;)
Created attachment 277209 [details] [review] Patch contains draft for tests implementation Unused parts were deleted and Alejandro comments applied. I think now it is more clear and much better.
Created attachment 277210 [details] [review] Patch contains draft for tests implementation Unused parts were deleted and Alejandro comments applied. I think now it is more clear and much better.
Review of attachment 277210 [details] [review]: Thanks for the patch. Sorry for the delay on the review. As far as I see, now this patch is smaller as remove some of the tests, and only includes what is is needed (for example: a dummy AtkText implementation is not added). Is that correct? ::: atspi-tests/accessible-app.c @@ +34,3 @@ + ss = atk_object_ref_state_set(ATK_OBJECT(root_accessible)); + atk_state_set_add_states(ss, states, 5); + g_object_unref(G_OBJECT(ss)); What is the purpose of asking for the state_set of this object and adding some states there, if the state_set will be freed just after adding those states? ::: atspi-tests/dummyatk/my-atk-object.c @@ +69,3 @@ + printf("ref_child: Incorrect index of child.\n"); + return NULL; + } Instead of this manual showing of the error, on glib is advisable to use g_return_if_fail or g_return_val_if_fail. @@ +89,3 @@ + break; + } + if(i < 0)printf("Something wrong in parent-child strucutre.\n"); g_error instead of printf @@ +132,3 @@ +static void my_atk_object_class_init(MyAtkObjectClass *my_class) +{ + my_class->parent.set_parent = my_atk_object_set_parent; You shouldn't access directly parent, but casing to AtkObjectClass. Something like: AtkObjectClass *object_class = ATK_OBJECT_CLASS (my_klass); object_class->set_parent = my_atk_object_set_parent; ::: atspi-tests/dummyatk/my-atk-object.h @@ +20,3 @@ + AtkAttributeSet *attributes; + GPtrArray* children; + gint id; Again: could all this be a private structure instead of publicly accessed? ::: atspi-tests/test-application.c @@ +96,3 @@ + ((TestModuleInit) init)((gchar *)tdpath); + +} As I asked on my previous review: any reason to use gmodule to specify and load the tests? And now I will elaborate that question a little. at-spi2-core is based on glib. glib has already some utility methods to implement tests: https://developer.gnome.org/glib/2.40/glib-Testing.html And 2 really significant glib-based projects are already using that test framework: https://git.gnome.org/browse/clutter/tree/tests (clutter adds an little wrapper to ensure that there is always an stage) https://git.gnome.org/browse/gtk+/tree/testsuite So, any reason to use a custom solution to define, load and execute the tests, instead of what other glib projects are using? ::: configure.ac @@ +64,3 @@ +if test -z "$GTK_MODULE_DIR"; then + GTK_MODULE_DIR=gtk-2.0/modules +fi It is not needed to re-add where the gtk-2.0 module is, as you are using now a call to atk_bridge_adaptor_init @@ +72,3 @@ + @<:@default=no@:>@])], + [build_tests=${withval}], + [build_tests=no]) Note: if you start from a clean repository (git clean -fdx) and you keep the default option (or use --with-atspi-tests=no) executing the configure, you get a compilation error: Making all in atspi-tests make[2]: Entering directory `/mnt/data/source/at-spi2-atk/atspi-tests' make[2]: *** No rule to make target `all'. Stop. make[2]: Leaving directory `/mnt/data/source/at-spi2-atk/atspi-tests' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/mnt/data/source/at-spi2-atk' make: *** [all] Error 2
Review of attachment 277210 [details] [review]: ::: atspi-tests/accessible-app.c @@ +35,3 @@ + atk_state_set_add_states(ss, states, 5); + g_object_unref(G_OBJECT(ss)); +} right now unused, deleted ::: atspi-tests/dummyatk/my-atk-object.c @@ +70,3 @@ + return NULL; + } + changed to glib messages @@ +89,3 @@ + break; + } + if(i < 0)printf("Something wrong in parent-child strucutre.\n"); changed to glib messages @@ +132,3 @@ +static void my_atk_object_class_init(MyAtkObjectClass *my_class) +{ + my_class->parent.set_parent = my_atk_object_set_parent; right, fixed ::: atspi-tests/dummyatk/my-atk-object.h @@ +21,3 @@ + GPtrArray* children; + gint id; +}; changed to private ::: atspi-tests/test-application.c @@ +96,3 @@ + ((TestModuleInit) init)((gchar *)tdpath); + +} deleted gmodule usage ::: configure.ac @@ +64,3 @@ +if test -z "$GTK_MODULE_DIR"; then + GTK_MODULE_DIR=gtk-2.0/modules +fi sure, old part of code @@ +102,3 @@ +fi +AM_CONDITIONAL([ATK_ENABLE_TESTS], [test "${want_atk_tests}" = "yes"]) + condition to adding subdir in Makefile added, fixed
Created attachment 278022 [details] [review] Newest change all Alejandro comments applied Please let me know if I should change test framework. Currently I had used unit test framework for C - check.
Review of attachment 278022 [details] [review]: Hi, finally today I found a moment to review again this patch. But more about reviewing the patch, about taking a look to both check and gtest. Although check has been around longer, gtest also offers a good amount of features, and his API is more glib-friendly. After all, it is part of glib. And except some glib based modules that added a big amount of tests before gtest being available (like gstreamer), most of the glib based projects are using gtest. Not only clutter and gtk+ as I mentioned on IRC, but others like cheese, colord, dconf, evolution, etc. Even webkitgtk is using it. Taking into account that we are still on the phase of adding the first tests, and as I said, gtest is part of glib (so one less dependency), and at-spi2-atk is a glib module, I think that we should go for the option of using gtest. Sorry for the bad news, as that means more work. Thanks for the work till now. I will not review the rest of the patch, because probably after the port to gtest a lot of it will change.
Created attachment 281688 [details] [review] check framework dependencies deleted, new test added Please find the attached patch, check test framework is changed to glib test framework
ups, I added changes to very old version, sorry. I will update that in a moment. Sorry
Created attachment 281863 [details] [review] check framework dependencies deleted Dear All, Please find attached patch with test migrated from check testing framework to glib testing. Best regards, Patrick
Created attachment 282879 [details] [review] Next tests added This patch contains more tests for Accessible module.
(In reply to comment #10) > Created an attachment (id=281863) [details] [review] > check framework dependencies deleted > Thanks for the patches. Sorry for the lack of answers. Im right now on holidays, I will review the patches when I got back home.
Comment on attachment 281863 [details] [review] check framework dependencies deleted Thanks for the reminder, and I apologize for never reviewing it. It looks good over all. Just a few comments: +++ b/atspi-tests/atk_suite.h +#define ATK_TEST_PATH_MAIN (const char *)"/Accessiblity" Typo. +++ b/atspi-tests/atk_test_accessible.c + sleep(1); + run_app(); + int i; I'm concerned about whether it's portable to declare variables in the middle of a function in C code, although I honestly have no idea whether I need to be or not. + g_test_add_func( ATK_TEST_PATH_MAIN "/atk_test_accesible_name", atk_test_accessible_name ); Typo. Also, in general, you have code at the end of many of your tests to kill a child process, and I think it would be better to put this in a teardown function. It would at least avoid needing to repeat the code. See g_test_add_vtable, for instance. You could define a common teardown function and write a convenience function that would pass the appropriate parameters to g_test_add_vtable.
Comment on attachment 282879 [details] [review] Next tests added +void atk_test_accessible_get_parent( void ) +{ + AtspiAccessible *obj = get_root_obj(); + AtspiAccessible *child = atspi_accessible_get_child_at_index( obj, 0, NULL ); + AtspiAccessible *parent = atspi_accessible_get_parent( child, NULL ); + g_assert_cmpstr( atspi_accessible_get_name( parent, NULL ), ==, atspi_accessible_get_name( obj, NULL ) ); + kill(child_pid, SIGTERM); +} I'd prefer an assert that parent == obj; it would be good to test that AT-SPI returns a reference to the existing object, rather than creating a new object and making comparisons fail. Also, re the kill, it would be good to have this in a teardown function (see previous comment). +void atk_test_accessible_get_relation_set_2( void ) ... + //error which should be fixed + g_assert_cmpint( atspi_relation_get_relation_type(a), == , 3); ?? Should this be ATSPI_RELATION_CONTROLLER_FOR? Also, most modules have a directory simply named "tests," so I think it makes sense for the directory to be renamed that, rather than atspi-tests. I'm guessing that you named the directory "atspi-tests" because there was already a tests directory, but that directory should not have been there and has been removed (see https://bugzilla.gnome.org/show_bug.cgi?id=738000). But renaming the directory after these tests are committed would be fine with me if that's easiest. Anyway, thanks for your work!
Review of attachment 281863 [details] [review]: I didn't have time to made a full review, thanks to Mike to do it. Anyway, from my last skim on the bug (when I created bug 73800) I found some stuff that it would be good to add here. ::: Makefile.am @@ +3,3 @@ +if ATSPI_TESTS + +SUBDIRS += atspi-tests Now that bug 738000 was closed, the directory name for the tests can be just 'tests' ::: atspi-tests/atk_suite.c @@ +27,3 @@ + + itr = atc; + fputs("Available Test Cases:\n", stderr); Why the list of available test cases are sent to stderr? @@ +29,3 @@ + fputs("Available Test Cases:\n", stderr); + for (; itr->test_case; itr++) + fprintf(stderr, "\t%s\n", itr->test_case); why sometimes fputs is used and other fprintf? And why not g_print (or g_log, etc) is used? @@ +58,3 @@ + for (i = 1; i < argc; i++) + if ((strcmp(argv[i], "-h") == 0) || + (strcmp(argv[i], "--help") == 0)) This executable parses manually argc and argv, but test-application uses glib (GOptionEntry) to do that. What is the reason of this difference? @@ +60,3 @@ + (strcmp(argv[i], "--help") == 0)) + { + fprintf(stderr, "Usage:\n\t%s [test_case1 .. [test_caseN]]\n", As mentioned before: why the usage of the application is sent to stderr? ::: atspi-tests/atk_test_accessible.c @@ +31,3 @@ + AtspiAccessible *obj= NULL; + + sleep(1); Why this sleep is needed? I guess that it is needed in order to wait to the forked application to be available. If that is the case, I think that it needs a comment to explain it. ::: atspi-tests/data/test.xml @@ +1,3 @@ +<?xml version="1.0" ?> +<accessible description="Root of the accessible tree" name="root_object" role="1"> + <accessible description="first child" name="obj1" role="2"/> Having numbers as roles make it hard to read the xml files (that is in theory one of the advantages). I would prefer to use the ATK role name and then use atk_role_for_name() in order to get the role at xml-loader.c ::: atspi-tests/dummyatk/my-atk-object.c @@ +130,3 @@ + b->value = g_strdup("qux"); + c->name = g_strdup("quux"); + c->value = g_strdup("corge"); What is the rationale of having attributes with random names? @@ +139,3 @@ +static void my_atk_object_init (MyAtkObject *self) +{ + self->children = g_ptr_array_sized_new(10); Probably using the _full option would be better. In that way you can specificy a destroy function, so you would not need to manually do an _unref each time you remove an item, like at other places of the code. ::: atspi-tests/test-application.c @@ +30,3 @@ + * provide its own implementation of atk_get_root, and as such provide + * all the application state for the test. + */ This text is really informative. But it is only available on the source code. I think that the test suite would need some kind of documentation. If not a full documentation with several details and how to write new tests, at least having the text of this comment and some extra details on a README file. This is more important here, as some of the binaries are not supposed to be used directly, but utility to the real test suite (that is a long way to say that it is not clear how to execute the tests) You can see clutter one as a reference: https://git.gnome.org/browse/clutter/tree/tests/README @@ +36,3 @@ +#include <stdlib.h> +#include <glib.h> +#include <gmodule.h> As far as I see, gmodule is not used anymore @@ +59,3 @@ + + if (path == NULL) + g_error("No test data path provided"); At this point, if failing, the application just crash. I think that it would be just exit (without a crash). In general this application it is not suited to be executed manually. If an user tries to execute it manually, and fails, probably it would be good to mention it (also see my comment about the need of a README file). ::: configure.ac @@ +63,3 @@ + [AC_HELP_STRING([--with-atspi-tests=yes|no], + [choose if atk test should be enabled. + @<:@default=no@:>@])], This is somewhat confusing. The option is called "with-atspi-tests" but the description says "choose if *atk* test should be enabled". I woouls simplify and just use "--with-tests" and "choose if tests should be enabled". @@ +67,3 @@ + [build_tests=no]) + +want_atk_tests="no" Under the same rationale, this would need to be "want_tests" @@ +86,3 @@ + AC_SUBST(XML_CFLAGS) + + PKG_CHECK_MODULES(DBUS_GLIB, [dbus-glib-1 >= 0.100.2]) Is really dbus-glib needed?
Created attachment 289462 [details] [review] Unit tests New patch, all comments applied. I had also merged two last patches to make review easier. @API, you had asked about 3 different attributes name. I do not have found any rationale reason to use that in this way but I think it is still some kind of test, but If you want I can delete that.
Review of attachment 289462 [details] [review]: In general looks good. Almost everything I found is related with code-style (mostly about being consistent), and some minor stuff. It is almost ready. Sorry if it is taking long, but as this commit is also adding the test suite, it is also the bigger/more complex. I hope that following patches, that would just add specific tests, would be easier to get into. ::: configure.ac @@ +67,3 @@ + [build_tests=no]) + +want_atk_tests="no" Minor: why not just "want_tests"? ::: tests/README @@ +33,3 @@ + c) Create tested functions + d) Create function which calls all test functions, this function should be called in atk_suite.c file. + Thanks for the README file. Now everything is more understandable. But I miss one thing. As far as I see, this commit adds the test suite, and the first test for that suite. That is atk_test_accessible. What I miss is the explanation of that specific test, and a list of available test. The idea is that if someone wants to add a new test, there would be a place to read which ones are already available. ::: tests/atk-object-xml-loader.h @@ +25,3 @@ + +MyAtkObject * +atk_object_xml_parse(gchar *filename); code-style: on headers, usually this goes on the same line. ::: tests/atk_test_accessible.c @@ +37,3 @@ + run_app(); + + // sleep is needed to wait for fored test application code-style: please comments in c-style (I know that we have some of those on the code, but in any case). typo: s/fored/forked @@ +134,3 @@ + int i=0; + for( i = 0; i < rel_set->len; i++) + { nitpick: please, be consistent with the brackets. If you put the bracket on the next line then it should be indented (as in other places of at-spi2-atk). In any case, in the rest of your code, you put the bracket on the same line. Probably that would be better. ::: tests/data/test.xml @@ +1,2 @@ +<?xml version="1.0" ?> +<accessible description="Root of the accessible tree" name="root_object" role="accelerator label"> Note for the future: eventually we would like to have a tree "less synthetic", in the sense of having a hierarchy more similar to what we would find in real life. But for now, this hierarchy is ok. ::: tests/dummyatk/my-atk-object.c @@ +24,3 @@ +G_DEFINE_TYPE (MyAtkObject, my_atk_object, ATK_TYPE_OBJECT); + +void my_atk_object_add_child(MyAtkObject* parent, MyAtkObject* child) code-style: Again, each parameter on a different line. @@ +44,3 @@ + g_return_if_fail(i < 0); + + g_ptr_array_remove_index(parent->children, i); code style: a space between "index" and "(" is needed @@ +45,3 @@ + + g_ptr_array_remove_index(parent->children, i); + g_object_unref(child); You created the array with g_ptr_array_new_full, and specifying g_object_unref as the GDestroyNotify function. So this g_object_unref is not needed. @@ +53,3 @@ +{ + + g_return_if_fail(parent != NULL); Are you sure that the parent can't be NULL? What happens with the root object? @@ +64,3 @@ + klass->set_parent(accessible, parent); + + if(parent_old != NULL) code style: a space between "index" and "(" is needed (sometimes you do this, is just about being consistent). @@ +147,3 @@ +{ + self->children = g_ptr_array_new_full(10, g_object_unref); + self->attributes = g_slist_alloc(); g_slist_alloc is intended to be used by g_slist_insert/prepend/insert_sorted internally. Usually GSLists are initialized to NULL, that represents an empty list. ::: tests/dummyatk/my-atk-object.h @@ +16,3 @@ +GType my_atk_object_get_type(); + +void my_atk_object_add_child(MyAtkObject* parent, MyAtkObject* child); code-style: each parameter should be on a different line, so: void my_atk_object_add_child(MyAtkObject* parent, MyAtkObject* child); ::: tests/dummyatk/my-atk.h @@ +2,3 @@ +#define MY_ATK_H + +#include <my-atk.h> my-atk.h is including itself? ::: tests/test-application.c @@ +27,3 @@ + * This file contains the entry point for all test applications. + * Each test is built as a GModule, and this program loads the + * test module, as well as the AT-SPI module. The test module will This documentation is outdated, right? I mean that the tests are not built anymore as GModule modules, and the atspi is not loaded anymore with GModule either.
Created attachment 291382 [details] [review] Unit tests Many thanks for last review. I had fixed code style, now everything is consistent. I had applied your other suggestions. I'm looking forward for your review :) Thanks!
Created attachment 291457 [details] [review] Unit tests Many thanks for last review. I had fixed code style, now everything is consistent. I had applied your other suggestions. I'm looking forward for your review :) + One small fix Thanks!
Review of attachment 291457 [details] [review]: LGTM, and I think that it is in good shape to be committed. After all this is the starting point, any further improvements/changes can be done in the future. Just remember to add a meaningful commit message (including the link to the bug). Anyway, as Im not the maintainer of this module (I am the ATK maintainer), I will not set this patch as accepted yet. We will ping Mike Gorse on today meeting. ::: tests/atk-object-xml-loader.c @@ +1,3 @@ +/* + * AT-SPI - Assistive Technology Service Provider Interface + * (Gnome Accessibility Project; http://developer.gnome.org/projects/gap) This page doesn't exist anymore, please use this: https://wiki.gnome.org/Accessibility
Created attachment 291979 [details] [review] Unit tests Page address in header changed, thanks a lot for review.
Created attachment 291980 [details] [review] Unit tests Page address in header changed, thanks a lot for review.
Thanks for all the work. I'll take a look tonight. Do you have commit access, or should I commit it?
Mike as I mentioned on IRC, I do not have commit access, so please commit my task.
Comment on attachment 291980 [details] [review] Unit tests I apologize for the delay. Made a couple of minor changes to the README and committed. Thanks for the patch.