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

Issue 6267413888761856: Issue #1163 - add unit tests for CPluginUserSettings (Closed)

Created:
Aug. 6, 2014, 3:52 p.m. by Eric
Modified:
Dec. 31, 2014, 4:25 p.m.
Visibility:
Public.

Description

Issue #1163 - add unit tests for CPluginUserSettings Add some unit tests for CPluginUserSettings. These are the first unit tests for the plugin. Added a new project "tests_plugin" for these tests, since a significant fraction of the plugin code is required to resolve all symbols. This change is in preparation for refactoring the control flow of CPluginUserSettings.

Patch Set 1 : #

Total comments: 10

Patch Set 2 : #

Total comments: 7

Patch Set 3 : #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+204 lines, -0 lines) Patch
M adblockplus.gyp View 1 1 chunk +83 lines, -0 lines 0 comments Download
A test/plugin/UserSettingsTest.cpp View 1 2 1 chunk +121 lines, -0 lines 3 comments Download

Messages

Total messages: 9
Eric
All the GYP changes are in this change set. Having a place for plugin unit ...
Aug. 6, 2014, 4:59 p.m. (2014-08-06 16:59:59 UTC) #1
sergei
Beside comments below, the code looks good. There is nothing related to #1173. We should ...
Aug. 14, 2014, 11:37 a.m. (2014-08-14 11:37:26 UTC) #2
Eric
The relation to #1173 is that this test is a prerequisite to adding an exception ...
Aug. 14, 2014, 2:29 p.m. (2014-08-14 14:29:53 UTC) #3
Oleksandr
LGTM
Aug. 17, 2014, 11:01 p.m. (2014-08-17 23:01:53 UTC) #4
Felix Dahlke
http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/adblockplus.gyp File adblockplus.gyp (right): http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/adblockplus.gyp#newcode222 adblockplus.gyp:222: { Not really a fan of duplicating source listing, ...
Sept. 30, 2014, 1:50 p.m. (2014-09-30 13:50:59 UTC) #5
Eric
New change set contains nothing but code style changes. (Or at least it should.) http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/adblockplus.gyp ...
Oct. 7, 2014, 4:58 p.m. (2014-10-07 16:58:10 UTC) #6
Felix Dahlke
LGTM, please file the follow-up issue for cleaning up the build configuration. http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/adblockplus.gyp File adblockplus.gyp ...
Nov. 3, 2014, 4:21 a.m. (2014-11-03 04:21:56 UTC) #7
sergei
LGTM. Beside comments in code I would like to say that in most cases it's ...
Nov. 3, 2014, 2:41 p.m. (2014-11-03 14:41:36 UTC) #8
Felix Dahlke
Nov. 3, 2014, 4:05 p.m. (2014-11-03 16:05:17 UTC) #9
http://codereview.adblockplus.org/6267413888761856/diff/5700735861784576/test...
File test/plugin/UserSettingsTest.cpp (right):

http://codereview.adblockplus.org/6267413888761856/diff/5700735861784576/test...
test/plugin/UserSettingsTest.cpp:29: void SingleMethodNameFound(std::wstring
name, DISPID expected_id)
On 2014/11/03 14:41:36, sergei wrote:
> I think we need some well known abbreviation like CRIM = "const reference is
> missed".

I actually thought this is modified below and that's why it's passed by value.
On second glance it's read-only, so I fully agree with the CRIM.

Powered by Google App Engine
This is Rietveld