Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29370977: Issue #3593 - Stop sharing a single isolate amongst all unit tests

Jan. 10, 2017, 2:50 p.m. by Eric
Jan. 10, 2017, 7:03 p.m.
sergei, Felix Dahlke


Issue #3593 - Stop sharing a single isolate amongst all unit tests Change `CreateJsEngine()` in "BaseJsTest" to let `JsEngine::New()` allocate the isolate. Remove its local static variable to hold a shared isolate. Added checks in `TearDown()` for all unit tests to verify that the use count of `jsEngine` is zero. This entailed adding `TearDown()` routines if needed. Reworked memory allocation in unit tests to explicitly reset shared pointers in the `TearDown()` routines. Something about Google Test requires this to release memory when the test ends. This item required a rewrite in classes `PrefsTest` and `UpdateCheckTest`. Replace explicit `new` with `make_shared()` in a number of places. Rework `PrefsTest::Reset()` to create `JsValue` objects, rather than having the unit tests do it. The old method was incorrect without a shared isolate, because `JsValue` preferences were being created under the old isolate and then could not be used in the new isolate.

Patch Set 1 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+84 lines, -33 lines) Patch
M test/BaseJsTest.cpp View 2 chunks +3 lines, -2 lines 0 comments Download
M test/FilterEngine.cpp View 1 chunk +3 lines, -0 lines 0 comments Download
M test/Prefs.cpp View 6 chunks +49 lines, -16 lines 0 comments Download
M test/UpdateCheck.cpp View 3 chunks +29 lines, -15 lines 0 comments Download


Total messages: 1
Jan. 10, 2017, 7:03 p.m. (2017-01-10 19:03:40 UTC) #1
The unit test suite passes as a whole. On my machine, running a 32-bit build,
total memory consumption tops out at 44 Mb. (Verified through VS 2107 RC, which
has a live process memory graph while debugging.) There are still some leaks in
some of the tests that I haven't investigated; I don't know whether they're an
artifact of the tests and test framework or whether their in the units under

I'm very happy that the tests run entirely without engine leaks. There's a
verification point in the tests to ensure this, and I audited to the code to
make sure it was called everywhere.

This does not finish #3593, but it goes most of the way there. The last step
will be to remove the isolate argument from `JsEngine::New` that was itself a
workaround. Since that's part of the public API for the library, it deserves its
own change set.

Powered by Google App Engine
This is Rietveld