GNOME Bugzilla – Bug 691108
context: Only call JS_SetCStringsAreUTF8() once
Last modified: 2013-01-07 20:03:26 UTC
Otherwise, this fails some tests with debug builds of libmozjs. Unfortunately, this still fails with some unit tests that call JS_NewRuntime manually. Trying to make them gjs_context_new() ends with disaster, complaining about reregistering types.
Created attachment 232700 [details] [review] context: Only call JS_SetCStringsAreUTF8() once The JS engine mandates that this is called once, and before the first runtime is constructed. This is enforced by an assertion that's turned on in debug builds.
Review of attachment 232700 [details] [review]: ::: gjs/context.c @@ +563,3 @@ + JS_SetCStringsAreUTF8(); + new_runtime_called = TRUE; + } I would move this into a separate initialization helper, and maybe use GOnce just for consistency with other initializers.
(In reply to comment #0) > Otherwise, this fails some tests with debug builds of libmozjs. > Unfortunately, this still fails with some unit tests that call > JS_NewRuntime manually. Trying to make them gjs_context_new() > ends with disaster, complaining about reregistering types. The JS_SetCStringAreUTF8 call for that is in _gjs_unit_test_fixture_begin (gjs/unit-test-utils.c). You should be able to apply the same logic there.
I'd rather figure out why GTester is messing up. It makes sense to me to go through the same code path in unit tests that we do everywhere else.
(In reply to comment #4) > I'd rather figure out why GTester is messing up. It makes sense to me to go > through the same code path in unit tests that we do everywhere else. We're compiling the code with different flags to get at the test functions. If we also link to libgjs, we get duplicate symbols and thus wrong type registration.
Created attachment 232793 [details] [review] Fix GJS tests with debug builds of Spidermonkey SetCStringsAreUTF8 must be called only before any runtime is created Switching text fixtures to use GjsContext reveals that gjs-tests have been linking to libgjs all the time, despite exporting symbols with the same names. We must instead compile the libgjs sources separately. If you're interested, with this I completed a make check on debug js 185.
I was hoping to get rid of the crazy gjstests python file that generates a bunch of boilerplate we could manually write in each test file, because it's not enough code to manually write to want a code generator.
Created attachment 232917 [details] [review] unit-test-utils: Use a standard gjs context Rather than construct ourselves our own JSContext, use GjsContext, which will initialize the engine and call JS_SetCStringsAreUTF8(), which can only be called once.
Created attachment 232918 [details] [review] test: Refactor the unit test framework Rather than fight with a system which autogenerates a gtest for about ten tests, simply put tests manually in the file for now. By putting these tests in separate files, it also means that we won't have strange dual symbol issues by compiling the same source file twice. A test framework should represent usage in the real world, after all.
(In reply to comment #2) > Review of attachment 232700 [details] [review]: > > ::: gjs/context.c > @@ +563,3 @@ > + JS_SetCStringsAreUTF8(); > + new_runtime_called = TRUE; > + } > > I would move this into a separate initialization helper, and maybe use GOnce > just for consistency with other initializers. I started this, but expected you would say "lose the GOnce, we don't need thread safety here".
Review of attachment 232793 [details] [review]: I like my approach that removes the insanity better.
Created attachment 232921 [details] [review] context: Only call JS_SetCStringsAreUTF8() once The JS engine mandates that this is called once, and before the first runtime is constructed. This is enforced by an assertion that's turned on in debug builds.
Review of attachment 232918 [details] [review]: Two minor bits, feel free to commit after addressing. ::: test/gjs-tests.c @@ +27,3 @@ #include <glib.h> #include <glib-object.h> +#include <gjs/gjs-module.h> gjs-module.h should pull in jsapi.h, no? ::: test/test.h @@ +20,3 @@ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS * IN THE SOFTWARE. */ Shouldn't this file be deleted entirely now?
Review of attachment 232917 [details] [review]: Right.
Review of attachment 232921 [details] [review]: Looks right.
Attachment 232917 [details] pushed as 30d3fc0 - unit-test-utils: Use a standard gjs context Attachment 232918 [details] pushed as c4bd39e - test: Refactor the unit test framework Attachment 232921 [details] pushed as 9148ed0 - context: Only call JS_SetCStringsAreUTF8() once Thanks for spotting those -- I meant to delete test.h but I guess I forgot. Other things fixed.