Issue 2204051: Implement stress tool for ibus.
|
Created:
13 years, 6 months ago by hkobayash Modified:
13 years, 6 months ago CC:
Peng Huang, daigoh_chromium.org, daigoh_google.com Base URL:
git://github.com/ibus/ibus.git Visibility:
Public. |
DescriptionImplement stress tool for ibus.
A test-stress.c sends key message each time by client.
Client.h and client.c store modifier key and send key event.
They check ibus-daemon and engine is alive.
Patch Set 1 #Patch Set 2 : style fix #
Total comments: 57
Patch Set 3 : Fixed review #
Total comments: 4
Patch Set 4 : fixed review 2 #
MessagesTotal messages: 13
Kobayashi-san, Sorry for the delay. I have checked your code and left some comments. Please check them and revise your code. Hi Peng, Kobayashi-san, an intern in testing team in Tokyo, wrote a simple test (ibus/bus/test-stress.c) which sends a random key sequence to ibus-daemon and verify that the daemon and an engine do not crash. Do you think it's possible to add the test to the ibus upstream repository when the review finishes? If you have comments, please let Kobayashi-san know (his intern period ends on Oct 1).
Sign in to reply to this message.
Forgot to cc peng... resending. On 2010/09/29 07:07:59, Yusuke Sato wrote: > Kobayashi-san, > Sorry for the delay. I have checked your code and left some comments. Please > check them and revise your code. > > Hi Peng, > Kobayashi-san, an intern in testing team in Tokyo, wrote a simple test > (ibus/bus/test-stress.c) which sends a random key sequence to ibus-daemon and > verify that the daemon and an engine do not crash. Do you think it's possible to > add the test to the ibus upstream repository when the review finishes? If you > have comments, please let Kobayashi-san know (his intern period ends on Oct 1). http://codereview.appspot.com/2204051/diff/3001/bus/client.c File bus/client.c (right): http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode80 bus/client.c:80: // GObjectClass *gobject_class = G_OBJECT_CLASS (klass); can you remove the line? http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode92 bus/client.c:92: ibus_set_display (gdk_display_get_name (gdk_display_get_default ())); can't we use _xdisplay here? http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode114 bus/client.c:114: g_signal_connect (client->ibuscontext, Is it necessary to connect to the signal? The signal handler seems to do nothing. http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode122 bus/client.c:122: g_signal_connect (client->ibuscontext, ditto http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode126 bus/client.c:126: g_signal_connect (client->ibuscontext, ditto http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode130 bus/client.c:130: g_signal_connect (client->ibuscontext, ditto http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode140 bus/client.c:140: active_engine_name = _get_active_engine_name (); Your function, _get_active_engine_name, might return NULL if no engine is preloaded, or no engine is installed. Please handle the case correctly. http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode230 bus/client.c:230: for (i = 0; i < 7; i++) { please do not use magic numbers. #define BLAH_BLAH 7. http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode250 bus/client.c:250: g_list_free (engines); memory leak? I believe you have to unref each desc: GList *cursor; for (cursor = engines; cursor; cursor = g_list_next(cursor)) { g_object_unref(IBUS_ENGINE_DESC(cursor->data)); } http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode253 bus/client.c:253: static void one vertical space pls (to be consistent with other functions in the file.) http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode297 bus/client.c:297: static gint16 one vertical space pls. http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode370 bus/client.c:370: static guint one vertical space pls. http://codereview.appspot.com/2204051/diff/3001/bus/client.h File bus/client.h (right): http://codereview.appspot.com/2204051/diff/3001/bus/client.h#newcode1 bus/client.h:1: /* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */ These file are NOT part of ibus-daemon right? I think it's better to rename them to test-client.[ch] or something to clarify the files are just for testing. http://codereview.appspot.com/2204051/diff/3001/bus/client.h#newcode49 bus/client.h:49: struct _BusClient { Please add a comment that explains what the glib class does. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c File bus/test-stress.c (right): http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode29 bus/test-stress.c:29: static double Please do not reinvent the wheel. I think you can use: http://library.gnome.org/devel/glib/unstable/glib-Timers.html http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 bus/test-stress.c:51: gint please add a comment that explains what the main function tests. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 bus/test-stress.c:51: gint I think the test can not run if ibus-daemon is not running or no engines are preloaded. If so, please add a comment for the restriction as well. It's nice if you could relax the restriction, of course, though. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 bus/test-stress.c:51: gint Question: can the test detect a crash of ibus-daemon or ibus engines? http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode56 bus/test-stress.c:56: int count = 0; what is the variable for? pls add comment. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode62 bus/test-stress.c:62: srand(time(NULL)); Shouldn't we print the seed so that we can reproduce a crash? http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode62 bus/test-stress.c:62: srand(time(NULL)); you can use: http://library.gnome.org/devel/glib/unstable/glib-Random-Numbers.html rand() function in older Linux distributions that uses naive linear congruential method would be bad for this kind of usage (see man 3 rand). glib version would be better since it uses MT. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode68 bus/test-stress.c:68: if (!bus_client_is_operated (client)) { So, how can we stop the test? I guess we have to press Control+C or have to kill ibus-daemon explicitly, which are not convenient for automatic testing. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode72 bus/test-stress.c:72: if(count>0 || rand ()%5 == 0) { as I wrote in client.c, pls do not use magic numbers. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode85 bus/test-stress.c:85: _sleep(10); Please explain why we need to sleep here, and why 10. I don't think 0.1 qps test is a "stress" test :) http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode91 bus/test-stress.c:91: gtk_main (); is this line necessary? as I wrote above, I think the test should automatically exit (i.e. without a user interaction like Control+c.)
Sign in to reply to this message.
It has some problem when link with gold linker. CCLD test-stress /usr/bin/ld: test_stress-client.o: in function bus_client_class_init:client.c(.text+0x135): error: undefined reference to 'XOpenDisplay' /usr/bin/ld: test_stress-client.o: in function _get_keysym_to_keycode:client.c(.text+0x79e): error: undefined reference to 'XKeysymToKeycode' collect2: ld returned 1 exit status make[2]: *** [test-stress] Error 1 make[2]: Leaving directory `/usr/local/google/sources/ibus/ibus/bus' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/usr/local/google/sources/ibus/ibus' make: *** [all] Error 2 On 2010/09/29 07:08:55, Yusuke Sato wrote: > Forgot to cc peng... resending. > > On 2010/09/29 07:07:59, Yusuke Sato wrote: > > Kobayashi-san, > > Sorry for the delay. I have checked your code and left some comments. Please > > check them and revise your code. > > > > Hi Peng, > > Kobayashi-san, an intern in testing team in Tokyo, wrote a simple test > > (ibus/bus/test-stress.c) which sends a random key sequence to ibus-daemon and > > verify that the daemon and an engine do not crash. Do you think it's possible > to > > add the test to the ibus upstream repository when the review finishes? If you > > have comments, please let Kobayashi-san know (his intern period ends on Oct > 1). > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c > File bus/client.c (right): > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode80 > bus/client.c:80: // GObjectClass *gobject_class = G_OBJECT_CLASS (klass); > can you remove the line? > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode92 > bus/client.c:92: ibus_set_display (gdk_display_get_name (gdk_display_get_default > ())); > can't we use _xdisplay here? > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode114 > bus/client.c:114: g_signal_connect (client->ibuscontext, > Is it necessary to connect to the signal? The signal handler seems to do > nothing. > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode122 > bus/client.c:122: g_signal_connect (client->ibuscontext, > ditto > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode126 > bus/client.c:126: g_signal_connect (client->ibuscontext, > ditto > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode130 > bus/client.c:130: g_signal_connect (client->ibuscontext, > ditto > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode140 > bus/client.c:140: active_engine_name = _get_active_engine_name (); > Your function, _get_active_engine_name, might return NULL if no engine is > preloaded, or no engine is installed. > > Please handle the case correctly. > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode230 > bus/client.c:230: for (i = 0; i < 7; i++) { > please do not use magic numbers. > #define BLAH_BLAH 7. > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode250 > bus/client.c:250: g_list_free (engines); > memory leak? I believe you have to unref each desc: > > GList *cursor; > for (cursor = engines; cursor; cursor = g_list_next(cursor)) { > g_object_unref(IBUS_ENGINE_DESC(cursor->data)); > } > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode253 > bus/client.c:253: static void > one vertical space pls (to be consistent with other functions in the file.) > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode297 > bus/client.c:297: static gint16 > one vertical space pls. > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode370 > bus/client.c:370: static guint > one vertical space pls. > > http://codereview.appspot.com/2204051/diff/3001/bus/client.h > File bus/client.h (right): > > http://codereview.appspot.com/2204051/diff/3001/bus/client.h#newcode1 > bus/client.h:1: /* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */ > These file are NOT part of ibus-daemon right? > I think it's better to rename them to test-client.[ch] or something to clarify > the files are just for testing. > > http://codereview.appspot.com/2204051/diff/3001/bus/client.h#newcode49 > bus/client.h:49: struct _BusClient { > Please add a comment that explains what the glib class does. > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c > File bus/test-stress.c (right): > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode29 > bus/test-stress.c:29: static double > Please do not reinvent the wheel. I think you can use: > http://library.gnome.org/devel/glib/unstable/glib-Timers.html > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 > bus/test-stress.c:51: gint > please add a comment that explains what the main function tests. > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 > bus/test-stress.c:51: gint > I think the test can not run if ibus-daemon is not running or no engines are > preloaded. If so, please add a comment for the restriction as well. > > It's nice if you could relax the restriction, of course, though. > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 > bus/test-stress.c:51: gint > Question: can the test detect a crash of ibus-daemon or ibus engines? > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode56 > bus/test-stress.c:56: int count = 0; > what is the variable for? pls add comment. > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode62 > bus/test-stress.c:62: srand(time(NULL)); > Shouldn't we print the seed so that we can reproduce a crash? > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode62 > bus/test-stress.c:62: srand(time(NULL)); > you can use: > http://library.gnome.org/devel/glib/unstable/glib-Random-Numbers.html > > rand() function in older Linux distributions that uses naive linear congruential > method would be bad for this kind of usage (see man 3 rand). glib version would > be better since it uses MT. > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode68 > bus/test-stress.c:68: if (!bus_client_is_operated (client)) { > So, how can we stop the test? I guess we have to press Control+C or have to kill > ibus-daemon explicitly, which are not convenient for automatic testing. > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode72 > bus/test-stress.c:72: if(count>0 || rand ()%5 == 0) { > as I wrote in client.c, pls do not use magic numbers. > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode85 > bus/test-stress.c:85: _sleep(10); > Please explain why we need to sleep here, and why 10. > I don't think 0.1 qps test is a "stress" test :) > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode91 > bus/test-stress.c:91: gtk_main (); > is this line necessary? as I wrote above, I think the test should automatically > exit (i.e. without a user interaction like Control+c.)
Sign in to reply to this message.
Peng, thanks for the comment. hkobayashi: Probably you have to explicitly add -lX11 in Makefile.am. --Yusuke On Wed, Sep 29, 2010 at 5:12 PM, <Shawn.P.Huang@gmail.com> wrote: > It has some problem when link with gold linker. > > CCLD test-stress > /usr/bin/ld: test_stress-client.o: in function > bus_client_class_init:client.c(.text+0x135): error: undefined reference > to 'XOpenDisplay' > /usr/bin/ld: test_stress-client.o: in function > _get_keysym_to_keycode:client.c(.text+0x79e): error: undefined reference > to 'XKeysymToKeycode' > collect2: ld returned 1 exit status > make[2]: *** [test-stress] Error 1 > make[2]: Leaving directory `/usr/local/google/sources/ibus/ibus/bus' > make[1]: *** [all-recursive] Error 1 > make[1]: Leaving directory `/usr/local/google/sources/ibus/ibus' > make: *** [all] Error 2 > > > > On 2010/09/29 07:08:55, Yusuke Sato wrote: > >> Forgot to cc peng... resending. >> > > On 2010/09/29 07:07:59, Yusuke Sato wrote: >> > Kobayashi-san, >> > Sorry for the delay. I have checked your code and left some >> > comments. Please > >> > check them and revise your code. >> > >> > Hi Peng, >> > Kobayashi-san, an intern in testing team in Tokyo, wrote a simple >> > test > >> > (ibus/bus/test-stress.c) which sends a random key sequence to >> > ibus-daemon and > >> > verify that the daemon and an engine do not crash. Do you think it's >> > possible > >> to >> > add the test to the ibus upstream repository when the review >> > finishes? If you > >> > have comments, please let Kobayashi-san know (his intern period ends >> > on Oct > >> 1). >> > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c >> File bus/client.c (right): >> > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode80 >> bus/client.c:80: // GObjectClass *gobject_class = G_OBJECT_CLASS >> > (klass); > >> can you remove the line? >> > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode92 >> bus/client.c:92: ibus_set_display (gdk_display_get_name >> > (gdk_display_get_default > >> ())); >> can't we use _xdisplay here? >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode114 > >> bus/client.c:114: g_signal_connect (client->ibuscontext, >> Is it necessary to connect to the signal? The signal handler seems to >> > do > >> nothing. >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode122 > >> bus/client.c:122: g_signal_connect (client->ibuscontext, >> ditto >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode126 > >> bus/client.c:126: g_signal_connect (client->ibuscontext, >> ditto >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode130 > >> bus/client.c:130: g_signal_connect (client->ibuscontext, >> ditto >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode140 > >> bus/client.c:140: active_engine_name = _get_active_engine_name (); >> Your function, _get_active_engine_name, might return NULL if no engine >> > is > >> preloaded, or no engine is installed. >> > > Please handle the case correctly. >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode230 > >> bus/client.c:230: for (i = 0; i < 7; i++) { >> please do not use magic numbers. >> #define BLAH_BLAH 7. >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode250 > >> bus/client.c:250: g_list_free (engines); >> memory leak? I believe you have to unref each desc: >> > > GList *cursor; >> for (cursor = engines; cursor; cursor = g_list_next(cursor)) { >> g_object_unref(IBUS_ENGINE_DESC(cursor->data)); >> } >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode253 > >> bus/client.c:253: static void >> one vertical space pls (to be consistent with other functions in the >> > file.) > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode297 > >> bus/client.c:297: static gint16 >> one vertical space pls. >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode370 > >> bus/client.c:370: static guint >> one vertical space pls. >> > > http://codereview.appspot.com/2204051/diff/3001/bus/client.h >> File bus/client.h (right): >> > > http://codereview.appspot.com/2204051/diff/3001/bus/client.h#newcode1 >> bus/client.h:1: /* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: >> > nil; -*- */ > >> These file are NOT part of ibus-daemon right? >> I think it's better to rename them to test-client.[ch] or something to >> > clarify > >> the files are just for testing. >> > > http://codereview.appspot.com/2204051/diff/3001/bus/client.h#newcode49 >> bus/client.h:49: struct _BusClient { >> Please add a comment that explains what the glib class does. >> > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c >> File bus/test-stress.c (right): >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode29 > >> bus/test-stress.c:29: static double >> Please do not reinvent the wheel. I think you can use: >> http://library.gnome.org/devel/glib/unstable/glib-Timers.html >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 > >> bus/test-stress.c:51: gint >> please add a comment that explains what the main function tests. >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 > >> bus/test-stress.c:51: gint >> I think the test can not run if ibus-daemon is not running or no >> > engines are > >> preloaded. If so, please add a comment for the restriction as well. >> > > It's nice if you could relax the restriction, of course, though. >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 > >> bus/test-stress.c:51: gint >> Question: can the test detect a crash of ibus-daemon or ibus engines? >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode56 > >> bus/test-stress.c:56: int count = 0; >> what is the variable for? pls add comment. >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode62 > >> bus/test-stress.c:62: srand(time(NULL)); >> Shouldn't we print the seed so that we can reproduce a crash? >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode62 > >> bus/test-stress.c:62: srand(time(NULL)); >> you can use: >> http://library.gnome.org/devel/glib/unstable/glib-Random-Numbers.html >> > > rand() function in older Linux distributions that uses naive linear >> > congruential > >> method would be bad for this kind of usage (see man 3 rand). glib >> > version would > >> be better since it uses MT. >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode68 > >> bus/test-stress.c:68: if (!bus_client_is_operated (client)) { >> So, how can we stop the test? I guess we have to press Control+C or >> > have to kill > >> ibus-daemon explicitly, which are not convenient for automatic >> > testing. > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode72 > >> bus/test-stress.c:72: if(count>0 || rand ()%5 == 0) { >> as I wrote in client.c, pls do not use magic numbers. >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode85 > >> bus/test-stress.c:85: _sleep(10); >> Please explain why we need to sleep here, and why 10. >> I don't think 0.1 qps test is a "stress" test :) >> > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode91 > >> bus/test-stress.c:91: gtk_main (); >> is this line necessary? as I wrote above, I think the test should >> > automatically > >> exit (i.e. without a user interaction like Control+c.) >> > > > > http://codereview.appspot.com/2204051/ >
Sign in to reply to this message.
hkobayashi: ping? If you have any questions, feel free to ask in chat/gmail. On 2010/09/29 08:15:49, Yusuke Sato wrote: > Peng, thanks for the comment. > > hkobayashi: > Probably you have to explicitly add -lX11 in Makefile.am. > > --Yusuke > > On Wed, Sep 29, 2010 at 5:12 PM, <mailto:Shawn.P.Huang@gmail.com> wrote: > > > It has some problem when link with gold linker. > > > > CCLD test-stress > > /usr/bin/ld: test_stress-client.o: in function > > bus_client_class_init:client.c(.text+0x135): error: undefined reference > > to 'XOpenDisplay' > > /usr/bin/ld: test_stress-client.o: in function > > _get_keysym_to_keycode:client.c(.text+0x79e): error: undefined reference > > to 'XKeysymToKeycode' > > collect2: ld returned 1 exit status > > make[2]: *** [test-stress] Error 1 > > make[2]: Leaving directory `/usr/local/google/sources/ibus/ibus/bus' > > make[1]: *** [all-recursive] Error 1 > > make[1]: Leaving directory `/usr/local/google/sources/ibus/ibus' > > make: *** [all] Error 2 > > > > > > > > On 2010/09/29 07:08:55, Yusuke Sato wrote: > > > >> Forgot to cc peng... resending. > >> > > > > On 2010/09/29 07:07:59, Yusuke Sato wrote: > >> > Kobayashi-san, > >> > Sorry for the delay. I have checked your code and left some > >> > > comments. Please > > > >> > check them and revise your code. > >> > > >> > Hi Peng, > >> > Kobayashi-san, an intern in testing team in Tokyo, wrote a simple > >> > > test > > > >> > (ibus/bus/test-stress.c) which sends a random key sequence to > >> > > ibus-daemon and > > > >> > verify that the daemon and an engine do not crash. Do you think it's > >> > > possible > > > >> to > >> > add the test to the ibus upstream repository when the review > >> > > finishes? If you > > > >> > have comments, please let Kobayashi-san know (his intern period ends > >> > > on Oct > > > >> 1). > >> > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c > >> File bus/client.c (right): > >> > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode80 > >> bus/client.c:80: // GObjectClass *gobject_class = G_OBJECT_CLASS > >> > > (klass); > > > >> can you remove the line? > >> > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode92 > >> bus/client.c:92: ibus_set_display (gdk_display_get_name > >> > > (gdk_display_get_default > > > >> ())); > >> can't we use _xdisplay here? > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode114 > > > >> bus/client.c:114: g_signal_connect (client->ibuscontext, > >> Is it necessary to connect to the signal? The signal handler seems to > >> > > do > > > >> nothing. > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode122 > > > >> bus/client.c:122: g_signal_connect (client->ibuscontext, > >> ditto > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode126 > > > >> bus/client.c:126: g_signal_connect (client->ibuscontext, > >> ditto > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode130 > > > >> bus/client.c:130: g_signal_connect (client->ibuscontext, > >> ditto > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode140 > > > >> bus/client.c:140: active_engine_name = _get_active_engine_name (); > >> Your function, _get_active_engine_name, might return NULL if no engine > >> > > is > > > >> preloaded, or no engine is installed. > >> > > > > Please handle the case correctly. > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode230 > > > >> bus/client.c:230: for (i = 0; i < 7; i++) { > >> please do not use magic numbers. > >> #define BLAH_BLAH 7. > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode250 > > > >> bus/client.c:250: g_list_free (engines); > >> memory leak? I believe you have to unref each desc: > >> > > > > GList *cursor; > >> for (cursor = engines; cursor; cursor = g_list_next(cursor)) { > >> g_object_unref(IBUS_ENGINE_DESC(cursor->data)); > >> } > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode253 > > > >> bus/client.c:253: static void > >> one vertical space pls (to be consistent with other functions in the > >> > > file.) > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode297 > > > >> bus/client.c:297: static gint16 > >> one vertical space pls. > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode370 > > > >> bus/client.c:370: static guint > >> one vertical space pls. > >> > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.h > >> File bus/client.h (right): > >> > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.h#newcode1 > >> bus/client.h:1: /* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: > >> > > nil; -*- */ > > > >> These file are NOT part of ibus-daemon right? > >> I think it's better to rename them to test-client.[ch] or something to > >> > > clarify > > > >> the files are just for testing. > >> > > > > http://codereview.appspot.com/2204051/diff/3001/bus/client.h#newcode49 > >> bus/client.h:49: struct _BusClient { > >> Please add a comment that explains what the glib class does. > >> > > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c > >> File bus/test-stress.c (right): > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode29 > > > >> bus/test-stress.c:29: static double > >> Please do not reinvent the wheel. I think you can use: > >> http://library.gnome.org/devel/glib/unstable/glib-Timers.html > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 > > > >> bus/test-stress.c:51: gint > >> please add a comment that explains what the main function tests. > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 > > > >> bus/test-stress.c:51: gint > >> I think the test can not run if ibus-daemon is not running or no > >> > > engines are > > > >> preloaded. If so, please add a comment for the restriction as well. > >> > > > > It's nice if you could relax the restriction, of course, though. > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 > > > >> bus/test-stress.c:51: gint > >> Question: can the test detect a crash of ibus-daemon or ibus engines? > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode56 > > > >> bus/test-stress.c:56: int count = 0; > >> what is the variable for? pls add comment. > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode62 > > > >> bus/test-stress.c:62: srand(time(NULL)); > >> Shouldn't we print the seed so that we can reproduce a crash? > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode62 > > > >> bus/test-stress.c:62: srand(time(NULL)); > >> you can use: > >> http://library.gnome.org/devel/glib/unstable/glib-Random-Numbers.html > >> > > > > rand() function in older Linux distributions that uses naive linear > >> > > congruential > > > >> method would be bad for this kind of usage (see man 3 rand). glib > >> > > version would > > > >> be better since it uses MT. > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode68 > > > >> bus/test-stress.c:68: if (!bus_client_is_operated (client)) { > >> So, how can we stop the test? I guess we have to press Control+C or > >> > > have to kill > > > >> ibus-daemon explicitly, which are not convenient for automatic > >> > > testing. > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode72 > > > >> bus/test-stress.c:72: if(count>0 || rand ()%5 == 0) { > >> as I wrote in client.c, pls do not use magic numbers. > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode85 > > > >> bus/test-stress.c:85: _sleep(10); > >> Please explain why we need to sleep here, and why 10. > >> I don't think 0.1 qps test is a "stress" test :) > >> > > > > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode91 > > > >> bus/test-stress.c:91: gtk_main (); > >> is this line necessary? as I wrote above, I think the test should > >> > > automatically > > > >> exit (i.e. without a user interaction like Control+c.) > >> > > > > > > > > http://codereview.appspot.com/2204051/ > >
Sign in to reply to this message.
Yusuke-san, thank you for your mail. Now I don't have no question and a few progress since I am busy to make a poster session document... -- Hirotake 2010年9月30日17:31 <yusukes@google.com>: > hkobayashi: ping? If you have any questions, feel free to ask in > chat/gmail. > > > On 2010/09/29 08:15:49, Yusuke Sato wrote: > >> Peng, thanks for the comment. >> > > hkobayashi: >> Probably you have to explicitly add -lX11 in Makefile.am. >> > > --Yusuke >> > > On Wed, Sep 29, 2010 at 5:12 PM, <mailto:Shawn.P.Huang@gmail.com> >> > wrote: > > > It has some problem when link with gold linker. >> > >> > CCLD test-stress >> > /usr/bin/ld: test_stress-client.o: in function >> > bus_client_class_init:client.c(.text+0x135): error: undefined >> > reference > >> > to 'XOpenDisplay' >> > /usr/bin/ld: test_stress-client.o: in function >> > _get_keysym_to_keycode:client.c(.text+0x79e): error: undefined >> > reference > >> > to 'XKeysymToKeycode' >> > collect2: ld returned 1 exit status >> > make[2]: *** [test-stress] Error 1 >> > make[2]: Leaving directory `/usr/local/google/sources/ibus/ibus/bus' >> > make[1]: *** [all-recursive] Error 1 >> > make[1]: Leaving directory `/usr/local/google/sources/ibus/ibus' >> > make: *** [all] Error 2 >> > >> > >> > >> > On 2010/09/29 07:08:55, Yusuke Sato wrote: >> > >> >> Forgot to cc peng... resending. >> >> >> > >> > On 2010/09/29 07:07:59, Yusuke Sato wrote: >> >> > Kobayashi-san, >> >> > Sorry for the delay. I have checked your code and left some >> >> >> > comments. Please >> > >> >> > check them and revise your code. >> >> > >> >> > Hi Peng, >> >> > Kobayashi-san, an intern in testing team in Tokyo, wrote a simple >> >> >> > test >> > >> >> > (ibus/bus/test-stress.c) which sends a random key sequence to >> >> >> > ibus-daemon and >> > >> >> > verify that the daemon and an engine do not crash. Do you think >> > it's > >> >> >> > possible >> > >> >> to >> >> > add the test to the ibus upstream repository when the review >> >> >> > finishes? If you >> > >> >> > have comments, please let Kobayashi-san know (his intern period >> > ends > >> >> >> > on Oct >> > >> >> 1). >> >> >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.c >> >> File bus/client.c (right): >> >> >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode80 > >> >> bus/client.c:80: // GObjectClass *gobject_class = G_OBJECT_CLASS >> >> >> > (klass); >> > >> >> can you remove the line? >> >> >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode92 > >> >> bus/client.c:92: ibus_set_display (gdk_display_get_name >> >> >> > (gdk_display_get_default >> > >> >> ())); >> >> can't we use _xdisplay here? >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode114 > >> > >> >> bus/client.c:114: g_signal_connect (client->ibuscontext, >> >> Is it necessary to connect to the signal? The signal handler seems >> > to > >> >> >> > do >> > >> >> nothing. >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode122 > >> > >> >> bus/client.c:122: g_signal_connect (client->ibuscontext, >> >> ditto >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode126 > >> > >> >> bus/client.c:126: g_signal_connect (client->ibuscontext, >> >> ditto >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode130 > >> > >> >> bus/client.c:130: g_signal_connect (client->ibuscontext, >> >> ditto >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode140 > >> > >> >> bus/client.c:140: active_engine_name = _get_active_engine_name (); >> >> Your function, _get_active_engine_name, might return NULL if no >> > engine > >> >> >> > is >> > >> >> preloaded, or no engine is installed. >> >> >> > >> > Please handle the case correctly. >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode230 > >> > >> >> bus/client.c:230: for (i = 0; i < 7; i++) { >> >> please do not use magic numbers. >> >> #define BLAH_BLAH 7. >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode250 > >> > >> >> bus/client.c:250: g_list_free (engines); >> >> memory leak? I believe you have to unref each desc: >> >> >> > >> > GList *cursor; >> >> for (cursor = engines; cursor; cursor = g_list_next(cursor)) { >> >> g_object_unref(IBUS_ENGINE_DESC(cursor->data)); >> >> } >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode253 > >> > >> >> bus/client.c:253: static void >> >> one vertical space pls (to be consistent with other functions in >> > the > >> >> >> > file.) >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode297 > >> > >> >> bus/client.c:297: static gint16 >> >> one vertical space pls. >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode370 > >> > >> >> bus/client.c:370: static guint >> >> one vertical space pls. >> >> >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.h >> >> File bus/client.h (right): >> >> >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.h#newcode1 > >> >> bus/client.h:1: /* -*- mode: C; c-basic-offset: 4; >> > indent-tabs-mode: > >> >> >> > nil; -*- */ >> > >> >> These file are NOT part of ibus-daemon right? >> >> I think it's better to rename them to test-client.[ch] or something >> > to > >> >> >> > clarify >> > >> >> the files are just for testing. >> >> >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/client.h#newcode49 > >> >> bus/client.h:49: struct _BusClient { >> >> Please add a comment that explains what the glib class does. >> >> >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c >> >> File bus/test-stress.c (right): >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode29 > >> > >> >> bus/test-stress.c:29: static double >> >> Please do not reinvent the wheel. I think you can use: >> >> http://library.gnome.org/devel/glib/unstable/glib-Timers.html >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 > >> > >> >> bus/test-stress.c:51: gint >> >> please add a comment that explains what the main function tests. >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 > >> > >> >> bus/test-stress.c:51: gint >> >> I think the test can not run if ibus-daemon is not running or no >> >> >> > engines are >> > >> >> preloaded. If so, please add a comment for the restriction as well. >> >> >> > >> > It's nice if you could relax the restriction, of course, though. >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 > >> > >> >> bus/test-stress.c:51: gint >> >> Question: can the test detect a crash of ibus-daemon or ibus >> > engines? > >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode56 > >> > >> >> bus/test-stress.c:56: int count = 0; >> >> what is the variable for? pls add comment. >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode62 > >> > >> >> bus/test-stress.c:62: srand(time(NULL)); >> >> Shouldn't we print the seed so that we can reproduce a crash? >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode62 > >> > >> >> bus/test-stress.c:62: srand(time(NULL)); >> >> you can use: >> >> >> > http://library.gnome.org/devel/glib/unstable/glib-Random-Numbers.html > >> >> >> > >> > rand() function in older Linux distributions that uses naive linear >> >> >> > congruential >> > >> >> method would be bad for this kind of usage (see man 3 rand). glib >> >> >> > version would >> > >> >> be better since it uses MT. >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode68 > >> > >> >> bus/test-stress.c:68: if (!bus_client_is_operated (client)) { >> >> So, how can we stop the test? I guess we have to press Control+C or >> >> >> > have to kill >> > >> >> ibus-daemon explicitly, which are not convenient for automatic >> >> >> > testing. >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode72 > >> > >> >> bus/test-stress.c:72: if(count>0 || rand ()%5 == 0) { >> >> as I wrote in client.c, pls do not use magic numbers. >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode85 > >> > >> >> bus/test-stress.c:85: _sleep(10); >> >> Please explain why we need to sleep here, and why 10. >> >> I don't think 0.1 qps test is a "stress" test :) >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode91 > >> > >> >> bus/test-stress.c:91: gtk_main (); >> >> is this line necessary? as I wrote above, I think the test should >> >> >> > automatically >> > >> >> exit (i.e. without a user interaction like Control+c.) >> >> >> > >> > >> > >> > http://codereview.appspot.com/2204051/ >> > >> > > > > http://codereview.appspot.com/2204051/ >
Sign in to reply to this message.
I finished to fix review and add linker option. http://codereview.appspot.com/2204051/diff/3001/bus/client.c File bus/client.c (right): http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode80 bus/client.c:80: // GObjectClass *gobject_class = G_OBJECT_CLASS (klass); On 2010/09/29 07:08:58, Yusuke Sato wrote: > can you remove the line? Done. http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode92 bus/client.c:92: ibus_set_display (gdk_display_get_name (gdk_display_get_default ())); On 2010/09/29 07:08:58, Yusuke Sato wrote: > can't we use _xdisplay here? Done. http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode114 bus/client.c:114: g_signal_connect (client->ibuscontext, On 2010/09/29 07:08:58, Yusuke Sato wrote: > Is it necessary to connect to the signal? The signal handler seems to do > nothing. It is debug http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode122 bus/client.c:122: g_signal_connect (client->ibuscontext, On 2010/09/29 07:08:58, Yusuke Sato wrote: > ditto Done. http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode126 bus/client.c:126: g_signal_connect (client->ibuscontext, On 2010/09/29 07:08:58, Yusuke Sato wrote: > ditto Done. http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode130 bus/client.c:130: g_signal_connect (client->ibuscontext, On 2010/09/29 07:08:58, Yusuke Sato wrote: > ditto Done. http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode140 bus/client.c:140: active_engine_name = _get_active_engine_name (); On 2010/09/29 07:08:58, Yusuke Sato wrote: > Your function, _get_active_engine_name, might return NULL if no engine is > preloaded, or no engine is installed. > > Please handle the case correctly. Done. http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode230 bus/client.c:230: for (i = 0; i < 7; i++) { On 2010/09/29 07:08:58, Yusuke Sato wrote: > please do not use magic numbers. > #define BLAH_BLAH 7. Done. http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode250 bus/client.c:250: g_list_free (engines); On 2010/09/29 07:08:58, Yusuke Sato wrote: > memory leak? I believe you have to unref each desc: > > GList *cursor; > for (cursor = engines; cursor; cursor = g_list_next(cursor)) { > g_object_unref(IBUS_ENGINE_DESC(cursor->data)); > } Done. http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode253 bus/client.c:253: static void On 2010/09/29 07:08:58, Yusuke Sato wrote: > one vertical space pls (to be consistent with other functions in the file.) Done. http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode297 bus/client.c:297: static gint16 On 2010/09/29 07:08:58, Yusuke Sato wrote: > one vertical space pls. Done. http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode370 bus/client.c:370: static guint On 2010/09/29 07:08:58, Yusuke Sato wrote: > one vertical space pls. Done. http://codereview.appspot.com/2204051/diff/3001/bus/client.h File bus/client.h (right): http://codereview.appspot.com/2204051/diff/3001/bus/client.h#newcode1 bus/client.h:1: /* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */ On 2010/09/29 07:08:58, Yusuke Sato wrote: > These file are NOT part of ibus-daemon right? > I think it's better to rename them to test-client.[ch] or something to clarify > the files are just for testing. Done. http://codereview.appspot.com/2204051/diff/3001/bus/client.h#newcode49 bus/client.h:49: struct _BusClient { On 2010/09/29 07:08:58, Yusuke Sato wrote: > Please add a comment that explains what the glib class does. Done. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c File bus/test-stress.c (right): http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode29 bus/test-stress.c:29: static double On 2010/09/29 07:08:58, Yusuke Sato wrote: > Please do not reinvent the wheel. I think you can use: > http://library.gnome.org/devel/glib/unstable/glib-Timers.html Done. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 bus/test-stress.c:51: gint On 2010/09/29 07:08:58, Yusuke Sato wrote: > please add a comment that explains what the main function tests. Done. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 bus/test-stress.c:51: gint On 2010/09/29 07:08:58, Yusuke Sato wrote: > I think the test can not run if ibus-daemon is not running or no engines are > preloaded. If so, please add a comment for the restriction as well. > > It's nice if you could relax the restriction, of course, though. Done. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode51 bus/test-stress.c:51: gint On 2010/09/29 07:08:58, Yusuke Sato wrote: > Question: can the test detect a crash of ibus-daemon or ibus engines? Test detect both. I made a separate of detect function(bus_test_client_is_connected and bus_test_client_is_enabled). http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode56 bus/test-stress.c:56: int count = 0; On 2010/09/29 07:08:58, Yusuke Sato wrote: > what is the variable for? pls add comment. Done. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode62 bus/test-stress.c:62: srand(time(NULL)); On 2010/09/29 07:08:58, Yusuke Sato wrote: > Shouldn't we print the seed so that we can reproduce a crash? Done. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode62 bus/test-stress.c:62: srand(time(NULL)); On 2010/09/29 07:08:58, Yusuke Sato wrote: > you can use: > http://library.gnome.org/devel/glib/unstable/glib-Random-Numbers.html > > rand() function in older Linux distributions that uses naive linear congruential > method would be bad for this kind of usage (see man 3 rand). glib version would > be better since it uses MT. Done. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode68 bus/test-stress.c:68: if (!bus_client_is_operated (client)) { On 2010/09/29 07:08:58, Yusuke Sato wrote: > So, how can we stop the test? I guess we have to press Control+C or have to kill > ibus-daemon explicitly, which are not convenient for automatic testing. Done. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode72 bus/test-stress.c:72: if(count>0 || rand ()%5 == 0) { On 2010/09/29 07:08:58, Yusuke Sato wrote: > as I wrote in client.c, pls do not use magic numbers. Done. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode85 bus/test-stress.c:85: _sleep(10); On 2010/09/29 07:08:58, Yusuke Sato wrote: > Please explain why we need to sleep here, and why 10. > I don't think 0.1 qps test is a "stress" test :) Because test-client receive signals and call back function. If no sleep, do not call back function. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode91 bus/test-stress.c:91: gtk_main (); On 2010/09/29 07:08:58, Yusuke Sato wrote: > is this line necessary? as I wrote above, I think the test should automatically > exit (i.e. without a user interaction like Control+c.) Done.
Sign in to reply to this message.
Thanks for the fix. http://codereview.appspot.com/2204051/diff/3001/bus/client.c File bus/client.c (right): http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode114 bus/client.c:114: g_signal_connect (client->ibuscontext, Then, please delete them. In general, we don't commit code for debugging. On 2010/10/01 02:20:58, hkobayash wrote: > On 2010/09/29 07:08:58, Yusuke Sato wrote: > > Is it necessary to connect to the signal? The signal handler seems to do > > nothing. > > It is debug http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c File bus/test-stress.c (right): http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode62 bus/test-stress.c:62: srand(time(NULL)); I don't think it's done. On 2010/10/01 02:20:58, hkobayash wrote: > On 2010/09/29 07:08:58, Yusuke Sato wrote: > > Shouldn't we print the seed so that we can reproduce a crash? > > Done. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode85 bus/test-stress.c:85: _sleep(10); 1 second sleep is still a problem. It slows down the test significantly. I think you should send keys to ibus-daemon probably in "commit-text" signal callback function like: _bus_commit_text_cb() { if (send_key_num > ...) { gtk_main_quit(); return; } bus_client_send_key (...); } main() { ... gtk_main(); } On 2010/10/01 02:20:58, hkobayash wrote: > On 2010/09/29 07:08:58, Yusuke Sato wrote: > > Please explain why we need to sleep here, and why 10. > > I don't think 0.1 qps test is a "stress" test :) > > Because test-client receive signals and call back function. > If no sleep, do not call back function. http://codereview.appspot.com/2204051/diff/15001/bus/test-client.c File bus/test-client.c (right): http://codereview.appspot.com/2204051/diff/15001/bus/test-client.c#newcode141 bus/test-client.c:141: g_return_if_fail (active_engine_name != NULL); Did you test the "active_engine_name is NULL" case? Can a user of your test see why the test does not work? http://codereview.appspot.com/2204051/diff/15001/bus/test-stress.c File bus/test-stress.c (right): http://codereview.appspot.com/2204051/diff/15001/bus/test-stress.c#newcode46 bus/test-stress.c:46: /* ibus stress test */ The comment explains almost nothing... Please explain what key sequences will be sent to the daemon, what the test function checks (e.g. ibus-daemon crash), etc.
Sign in to reply to this message.
Thank you for your review and fixed http://codereview.appspot.com/2204051/diff/3001/bus/client.c File bus/client.c (right): http://codereview.appspot.com/2204051/diff/3001/bus/client.c#newcode114 bus/client.c:114: g_signal_connect (client->ibuscontext, On 2010/10/01 03:41:46, yusukes wrote: > Then, please delete them. In general, we don't commit code for debugging. > > On 2010/10/01 02:20:58, hkobayash wrote: > > On 2010/09/29 07:08:58, Yusuke Sato wrote: > > > Is it necessary to connect to the signal? The signal handler seems to do > > > nothing. > > > > It is debug > Done. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c File bus/test-stress.c (right): http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode62 bus/test-stress.c:62: srand(time(NULL)); On 2010/10/01 03:41:46, yusukes wrote: > I don't think it's done. > > On 2010/10/01 02:20:58, hkobayash wrote: > > On 2010/09/29 07:08:58, Yusuke Sato wrote: > > > Shouldn't we print the seed so that we can reproduce a crash? > > > > Done. > Done. http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode85 bus/test-stress.c:85: _sleep(10); On 2010/10/01 03:41:46, yusukes wrote: > 1 second sleep is still a problem. It slows down the test significantly. > > I think you should send keys to ibus-daemon probably in "commit-text" signal > callback function like: > > _bus_commit_text_cb() { > if (send_key_num > ...) { gtk_main_quit(); return; } > bus_client_send_key (...); > } > > main() { > ... > gtk_main(); > } > > > > On 2010/10/01 02:20:58, hkobayash wrote: > > On 2010/09/29 07:08:58, Yusuke Sato wrote: > > > Please explain why we need to sleep here, and why 10. > > > I don't think 0.1 qps test is a "stress" test :) > > > > Because test-client receive signals and call back function. > > If no sleep, do not call back function. > It's not 1 second sleep but 1 milliseconds. http://codereview.appspot.com/2204051/diff/15001/bus/test-client.c File bus/test-client.c (right): http://codereview.appspot.com/2204051/diff/15001/bus/test-client.c#newcode141 bus/test-client.c:141: g_return_if_fail (active_engine_name != NULL); On 2010/10/01 03:41:47, yusukes wrote: > Did you test the "active_engine_name is NULL" case? > Can a user of your test see why the test does not work? My tool need to set active engine g_return_if_fail function print critical message. http://codereview.appspot.com/2204051/diff/15001/bus/test-stress.c File bus/test-stress.c (right): http://codereview.appspot.com/2204051/diff/15001/bus/test-stress.c#newcode46 bus/test-stress.c:46: /* ibus stress test */ On 2010/10/01 03:41:47, yusukes wrote: > The comment explains almost nothing... > > Please explain what key sequences will be sent to the daemon, what the test > function checks (e.g. ibus-daemon crash), etc. Done.
Sign in to reply to this message.
Add comment at 1 millisec sleep http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c File bus/test-stress.c (right): http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode85 bus/test-stress.c:85: _sleep(10); On 2010/10/01 06:15:56, hkobayash wrote: > On 2010/10/01 03:41:46, yusukes wrote: > > 1 second sleep is still a problem. It slows down the test significantly. > > > > I think you should send keys to ibus-daemon probably in "commit-text" signal > > callback function like: > > > > _bus_commit_text_cb() { > > if (send_key_num > ...) { gtk_main_quit(); return; } > > bus_client_send_key (...); > > } > > > > main() { > > ... > > gtk_main(); > > } > > > > > > > > On 2010/10/01 02:20:58, hkobayash wrote: > > > On 2010/09/29 07:08:58, Yusuke Sato wrote: > > > > Please explain why we need to sleep here, and why 10. > > > > I don't think 0.1 qps test is a "stress" test :) > > > > > > Because test-client receive signals and call back function. > > > If no sleep, do not call back function. > > > > It's not 1 second sleep but 1 milliseconds. If signal received, that event queues gtk event queue. And call back function is dispatched in time
Sign in to reply to this message.
On 2010/10/01 06:36:59, hkobayash wrote: > Add comment at 1 millisec sleep > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c > File bus/test-stress.c (right): > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode85 > bus/test-stress.c:85: _sleep(10); > On 2010/10/01 06:15:56, hkobayash wrote: > > On 2010/10/01 03:41:46, yusukes wrote: > > > 1 second sleep is still a problem. It slows down the test significantly. > > > > > > I think you should send keys to ibus-daemon probably in "commit-text" signal > > > callback function like: > > > > > > _bus_commit_text_cb() { > > > if (send_key_num > ...) { gtk_main_quit(); return; } > > > bus_client_send_key (...); > > > } > > > > > > main() { > > > ... > > > gtk_main(); > > > } > > > > > > > > > > > > On 2010/10/01 02:20:58, hkobayash wrote: > > > > On 2010/09/29 07:08:58, Yusuke Sato wrote: > > > > > Please explain why we need to sleep here, and why 10. > > > > > I don't think 0.1 qps test is a "stress" test :) > > > > > > > > Because test-client receive signals and call back function. > > > > If no sleep, do not call back function. > > > > > > > It's not 1 second sleep but 1 milliseconds. > > If signal received, that event queues gtk event queue. > And call back function is dispatched in time LGTM.
Sign in to reply to this message.
submitted. http://github.com/ibus/ibus/commit/bcfa545f0789573e805f721a953c6864f1f62a1b On 2010/10/01 09:28:34, Shawn.P.Huang wrote: > On 2010/10/01 06:36:59, hkobayash wrote: > > Add comment at 1 millisec sleep > > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c > > File bus/test-stress.c (right): > > > > http://codereview.appspot.com/2204051/diff/3001/bus/test-stress.c#newcode85 > > bus/test-stress.c:85: _sleep(10); > > On 2010/10/01 06:15:56, hkobayash wrote: > > > On 2010/10/01 03:41:46, yusukes wrote: > > > > 1 second sleep is still a problem. It slows down the test significantly. > > > > > > > > I think you should send keys to ibus-daemon probably in "commit-text" > signal > > > > callback function like: > > > > > > > > _bus_commit_text_cb() { > > > > if (send_key_num > ...) { gtk_main_quit(); return; } > > > > bus_client_send_key (...); > > > > } > > > > > > > > main() { > > > > ... > > > > gtk_main(); > > > > } > > > > > > > > > > > > > > > > On 2010/10/01 02:20:58, hkobayash wrote: > > > > > On 2010/09/29 07:08:58, Yusuke Sato wrote: > > > > > > Please explain why we need to sleep here, and why 10. > > > > > > I don't think 0.1 qps test is a "stress" test :) > > > > > > > > > > Because test-client receive signals and call back function. > > > > > If no sleep, do not call back function. > > > > > > > > > > It's not 1 second sleep but 1 milliseconds. > > > > If signal received, that event queues gtk event queue. > > And call back function is dispatched in time > > LGTM.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
