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

Issue 29372702: Issue #4826 - Use latch to replace thread-sleeping in 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:
Felix Dahlke, sergei
Visibility:
Public.

Description

Issue #4826 - Use latch to replace thread-sleeping in tests Add class `Latch` patterned after the Concurrency TS. It is not a complete version, but it's adequate for what we need for the unit tests. Add `JsTestingLatch` to expose a latch inside the v8 interpreter. This allows arriving at a latch in JS code and waiting on it in the C++ unit test code. Add some basic unit tests for the latch classes. Use the latch classes to replace `sleep_for` in unit tests where callbacks are available. The principle is to (1) instantiate a latch in the test (2) arrive at it in a callback, whether in C++ or JS, and (3) wait for it. This pattern eliminates all but one the race conditions in the tests that arise from the asynchronous behavior of file system I/O and web requests.

Patch Set 1 : #

Total comments: 3

Patch Set 2 : stray comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+489 lines, -75 lines) Patch
M libadblockplus.gyp View 2 chunks +5 lines, -0 lines 0 comments Download
A src/Latch.h View 1 chunk +96 lines, -0 lines 0 comments Download
A src/Latch.cpp View 1 chunk +48 lines, -0 lines 0 comments Download
M test/BaseJsTest.h View 2 chunks +2 lines, -0 lines 0 comments Download
M test/BaseJsTest.cpp View 1 chunk +2 lines, -1 line 0 comments Download
M test/FileSystemJsObject.cpp View 8 chunks +30 lines, -18 lines 0 comments Download
M test/FilterEngine.cpp View 3 chunks +7 lines, -7 lines 0 comments Download
M test/GlobalJsObject.cpp View 3 chunks +13 lines, -9 lines 0 comments Download
A test/JsLatch.h View 1 chunk +93 lines, -0 lines 0 comments Download
A test/JsLatch.cpp View 1 1 chunk +74 lines, -0 lines 0 comments Download
A test/LatchTest.cpp View 1 chunk +91 lines, -0 lines 0 comments Download
M test/Prefs.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M test/UpdateCheck.cpp View 8 chunks +15 lines, -21 lines 0 comments Download
M test/WebRequest.cpp View 6 chunks +11 lines, -17 lines 0 comments Download

Messages

Total messages: 1
Eric
2 years, 10 months ago (2017-01-19 18:38:07 UTC) #1
With this change set, all the unit tests run under the debugger without
crashing. Memory usage still maxes out at 21 MB, but it seems that total
(integrated) memory use is a bit lower. (I didn't formally measure it, so take
that with a grain of salt.)

This change set does not completely fix #4826, but almost so. The timeout tests
probably don't need fixing, that is, we can exclude them from #4826. The race in
the preference test is fundamental and needs separate attention.

https://codereview.adblockplus.org/29372702/diff/29372733/test/GlobalJsObject...
File test/GlobalJsObject.cpp (right):

https://codereview.adblockplus.org/29372702/diff/29372733/test/GlobalJsObject...
test/GlobalJsObject.cpp:32:
ASSERT_TRUE(jsEngine->Evaluate("this.foo")->IsUndefined()); // RACE
The race condition in these timeout tests is mostly benign. It would be useful
to have a test that verifies that timeouts do not fire prematurely, but writing
one correctly (without races) is quite involved.

Also, with the current implementation, it just pushes a race condition into the
thread scheduler. The timeout implementation would have to guarantee that
timeouts fire without rearrangement to be able to write a race-free test.

https://codereview.adblockplus.org/29372702/diff/29372733/test/Prefs.cpp
File test/Prefs.cpp (right):

https://codereview.adblockplus.org/29372702/diff/29372733/test/Prefs.cpp#newc...
test/Prefs.cpp:154: std::this_thread::sleep_for(std::chrono::milliseconds(100));
// RACE
The race condition here can't be eliminated with a callback in the test, since
there's no callback available. It related to #1582 about atomic file writes.

https://codereview.adblockplus.org/29372702/diff/29372733/test/WebRequest.cpp
File test/WebRequest.cpp (left):

https://codereview.adblockplus.org/29372702/diff/29372733/test/WebRequest.cpp...
test/WebRequest.cpp:68:
ASSERT_TRUE(jsEngine->Evaluate("this.foo")->IsUndefined());
I eliminated this test (and the sleep_for call above) because it's not doing
anything useful.
Sign in to reply to this message.

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