Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(80)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 10 months ago by Eric
Modified:
2 years, 10 months ago
Reviewers:
sergei, Felix Dahlke
Visibility:
Public.

Description

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

Messages

Total messages: 1
Eric
2 years, 10 months ago (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
test.

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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5