GNOME Bugzilla – Bug 630413
Patches for tests (xulrunner 1.9.3 prep work)
Last modified: 2010-09-23 18:17:56 UTC
Series of patches independent of xulrunner 1.9.3 work.
Created attachment 170909 [details] [review] tests: Don't timeout Having the process get aborted after a timeout is irritating for potentially long GDB sessions. Responsibility for handling potentially infinite hangs should be the job of a higher level test framework (for example, a cgroup or kvm), not randomly hardcoded inside individual tests.
Created attachment 170910 [details] [review] tests: Call g_type_init Might as well do this here instead of having random tests do it; we link to GObject anyways.
Created attachment 170911 [details] [review] tests: Disable JIT in tests Hopefully temporary. See: https://bugzilla.gnome.org/show_bug.cgi?id=616193
Created attachment 170912 [details] [review] tests: Refactor duplicate code for creating and destroying context For XULRunner 1.9.3 we'll need to fix this code, so ensure we only need to fix it in one place.
Created attachment 170913 [details] [review] tests: Add a test case for basic context creation/destruction Trying to narrow down a XULRunner 1.9.3 hang, and didn't see this simple test case anywhere else.
Created attachment 170914 [details] [review] tests: Add two basic tests These very simple js tests exercise the basic infrastructure, before we drop into more complex unit tests. Added for XULRunner 1.9.3 porting.
Created attachment 170915 [details] [review] tests: Make return code error more informative
Review of attachment 170909 [details] [review]: I would rather solve this by making it an env variable
Review of attachment 170910 [details] [review]: Looks fine to me
Review of attachment 170911 [details] [review]: OK to commit, since I don't have much of any how to do this better without a few days of debugging JIT internals, but can you attach it in the right place (to bug 616193) and remove the duplicate bug reference. Bug 616193 should be left open.
Review of attachment 170912 [details] [review]: Looks fine to me and like a good removal of duplicated code, except for one indentation problem. [The fancy version of this would be to be able to have: gjstest_test_func_gjs_jsapi_util_array(JSContext *context) And the extraction script would notice that and the wrapper code would pass in a context, but for three 5 tests, not worth that much work. ] ::: gjs/unit-test-utils.c @@ +36,3 @@ +_gjs_unit_test_fixture_begin (GjsUnitTestFixture *fixture) +{ + JSObject *global; Two space indents, doesn't match gjs style ::: gjs/unit-test-utils.h @@ +29,3 @@ +struct _GjsUnitTestFixture { + JSRuntime *runtime; + JSContext *context; Two space indents
Review of attachment 170913 [details] [review]: Mostly fine ::: gjs/context.c @@ +973,3 @@ + g_object_unref (context); + + context = gjs_context_new (); A comment about what you are trying to test by doing this twice would be good
Review of attachment 170914 [details] [review]: There's a basic problem with this test000N approach. If the order is meaningful, then future changes logically will require stuff to be added in between. And yes, you can add test00011 between test0001 and test0002 but then the thing with the 000 prefix gets confusing. Suggesting one of the following: - Add the tests add test1 test2 test3... - Add the tests as test0010 test0020 test0030 .... Also, it doesn't make sense to me to "run a basic test of dbus" (or any other add-on module - cairo, etc.) before we test the core. The mainloop test may make sense because it's testing parts of the core infrastructure - closures, etc. ::: test/js/test0002importer.js @@ +1,3 @@ +function testImporter1() { + var GLib = imports.gi.GLib; + log("GLib.E="+GLib.E); I'm of the opinion that tests should not log random crap. assert that GLib.MAXUINT16 is 0xffff or something ::: test/js/test0004mainloop.js @@ +10,3 @@ +/* A dangling timeout should get removed and not leaked */ +function testDanglingTimeout() { + Mainloop.timeout_add(5000, function() { log("this should not run"); }); That's only 5 seconds. Though I suggested a timeout before, I think I'd rather see an idle function that returns true as the dangler. Or up the timeout to an hour.
Review of attachment 170915 [details] [review]: Basically fine, trivial style thing ::: test/gjs-unit.c @@ +90,3 @@ g_error("%s", error->message); g_assert(error == NULL); + if (code != 0) { { for single line not matching a block two lines above
Attachment 170910 [details] pushed as 0aa0d50 - tests: Call g_type_init Attachment 170911 [details] pushed as 58ecae8 - tests: Disable JIT in tests Attachment 170912 [details] pushed as 5468db7 - tests: Refactor duplicate code for creating and destroying context Attachment 170913 [details] pushed as a7fe6a8 - tests: Add a test case for basic context creation/destruction Attachment 170915 [details] pushed as ae28484 - tests: Make return code error more informative
(In reply to comment #8) > Review of attachment 170909 [details] [review]: > > I would rather solve this by making it an env variable An environment variable to turn it on, or turn it off? Owen suggested detecting whether we're being ptraced currently. Dunno.