Issue 6267413888761856: Issue #1163 - add unit tests for CPluginUserSettings (Closed)
DescriptionIssue #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
MessagesTotal messages: 9
All the GYP changes are in this change set. Having a place for plugin unit tests is more important than the specific unit tests within this change set.
Beside comments below, the code looks good. There is nothing related to #1173. We should really as soon as possible decide whether we can use ATL or not. Using typelib and ATL::IDispatchImpl will save us from such hand made code and potential problems. I've left the comment in #1163 and in #276. http://codereview.adblockplus.org/6267413888761856/diff/5668600916475904/test... File test/plugin/UserSettingsTest.cpp (right): http://codereview.adblockplus.org/6267413888761856/diff/5668600916475904/test... test/plugin/UserSettingsTest.cpp:89: void invoke_invalid_dispatch_id(DISPID id) `id` is not used. http://codereview.adblockplus.org/6267413888761856/diff/5668600916475904/test... test/plugin/UserSettingsTest.cpp:92: DISPPARAMS params; DISPPARAMS params = {} looks better, but OK here. http://codereview.adblockplus.org/6267413888761856/diff/5668600916475904/test... test/plugin/UserSettingsTest.cpp:97: EXCEPINFO ex; I would also initialize it. http://codereview.adblockplus.org/6267413888761856/diff/5668600916475904/test... test/plugin/UserSettingsTest.cpp:98: HRESULT h = x.Invoke(-1, IID_NULL, 0, 0, ¶ms, nullptr, &ex, nullptr); The first arg should be `id`. http://codereview.adblockplus.org/6267413888761856/diff/5668600916475904/test... test/plugin/UserSettingsTest.cpp:98: HRESULT h = x.Invoke(-1, IID_NULL, 0, 0, ¶ms, nullptr, &ex, nullptr); Why is the flag 0? As I see we expose only methods, so we should use and check DISPATCH_METHOD. http://codereview.adblockplus.org/6267413888761856/diff/5668600916475904/test... test/plugin/UserSettingsTest.cpp:109: TEST(CPluginUserSettings_Invoke_Test, invalid_dispatch_id_underflow) I prefer more descriptive test names.
The relation to #1173 is that this test is a prerequisite to adding an exception handler in the patch set immediately following this one. But strictly speaking it doesn't need to be in this one. Note: they were originally in the same patch set; I split out the gyp modifications and the unit test. http://codereview.adblockplus.org/6267413888761856/diff/5668600916475904/test... File test/plugin/UserSettingsTest.cpp (right): http://codereview.adblockplus.org/6267413888761856/diff/5668600916475904/test... test/plugin/UserSettingsTest.cpp:89: void invoke_invalid_dispatch_id(DISPID id) On 2014/08/14 11:37:26, sergei wrote: > `id` is not used. Oops. Forgot to replace the -1 below when I refactored from the first version. http://codereview.adblockplus.org/6267413888761856/diff/5668600916475904/test... test/plugin/UserSettingsTest.cpp:92: DISPPARAMS params; On 2014/08/14 11:37:26, sergei wrote: > DISPPARAMS params = {} looks better, but OK here. Neither this nor the EXCEPINFO below are involved in the test. The only reason to have them here at all is to provide non-null pointers. If we need more extensive tests for this, we'll need non-trivial initializer functions for these anyway, and we can use them at that point. http://codereview.adblockplus.org/6267413888761856/diff/5668600916475904/test... test/plugin/UserSettingsTest.cpp:98: HRESULT h = x.Invoke(-1, IID_NULL, 0, 0, ¶ms, nullptr, &ex, nullptr); On 2014/08/14 11:37:26, sergei wrote: > Why is the flag 0? As I see we expose only methods, so we should use and check > DISPATCH_METHOD. I changed this in the test, but the larger problem is that the implementation of IDispatch under test doesn't check the flag at all. That's only a minor problem, because our JavaScript code doesn't try to manipulate properties on the "Settings" object, but I'm not going to fix it here; it's beyond the scope of the present unit of work. http://codereview.adblockplus.org/6267413888761856/diff/5668600916475904/test... test/plugin/UserSettingsTest.cpp:109: TEST(CPluginUserSettings_Invoke_Test, invalid_dispatch_id_underflow) On 2014/08/14 11:37:26, sergei wrote: > I prefer more descriptive test names. I added comments. If the test fails, the first thing you have to do is to look at the test code. I find the test names adequately descriptive.
LGTM
http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/adbl... File adblockplus.gyp (right): http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/adbl... adblockplus.gyp:222: { Not really a fan of duplicating source listing, include_dirs etc. in this target. I'm fine with this as a temporary thing, but we should at least file a follow-up issue for removing the duplication here. http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/test... File test/plugin/UserSettingsTest.cpp (right): http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/test... test/plugin/UserSettingsTest.cpp:29: void single_method_name_found(std::wstring name, DISPID expected_id) Camel case here and below please, coding style and all that... Also, how about turning name into a `const std::wstring&`? http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/test... test/plugin/UserSettingsTest.cpp:32: wchar_t *names[1]; Speaking of coding style, `wchar_t* ` here and below please. Also, how about using an array literal for brewity? wchar_t* names[] = {const_cast<wchar_t*>(name.c_str())}; http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/test... test/plugin/UserSettingsTest.cpp:37: DISPID id=ids[0]; Spaces around the = please.
New change set contains nothing but code style changes. (Or at least it should.) http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/adbl... File adblockplus.gyp (right): http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/adbl... adblockplus.gyp:222: { On 2014/09/30 13:50:59, Felix H. Dahlke wrote: > Not really a fan of duplicating source listing, include_dirs etc. in this > target. You noticed part of what was bothering me when I originally got this unit test module to compile, namely, that the tests themselves are only about a single class, and yet it takes eighteen extra non-header source files just to get the tests to link correctly. A few extra files wouldn't be all that surprising, but so many is indicative of larger issues in the structure of the code. > I'm fine with this as a temporary thing, but we should at least file a > follow-up issue for removing the duplication here. We certainly could, but I don't know what that issue would be, exactly. We could build a static library to house code common to the plugin and its tests, for example, or we could simple put common build elements in GYP variables. Nothing was obviously the right thing when I set this up, and that hasn't changed since. http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/test... File test/plugin/UserSettingsTest.cpp (right): http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/test... test/plugin/UserSettingsTest.cpp:32: wchar_t *names[1]; On 2014/09/30 13:50:59, Felix H. Dahlke wrote: > Also, how about using an array literal for brewity? > > wchar_t* names[] = {const_cast<wchar_t*>(name.c_str())}; It used to be that this wouldn't compile in Visual Studio. Done.
LGTM, please file the follow-up issue for cleaning up the build configuration. http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/adbl... File adblockplus.gyp (right): http://codereview.adblockplus.org/6267413888761856/diff/5750085036015616/adbl... adblockplus.gyp:222: { On 2014/10/07 16:58:10, Eric wrote: > On 2014/09/30 13:50:59, Felix H. Dahlke wrote: > > Not really a fan of duplicating source listing, include_dirs etc. in this > > target. > > You noticed part of what was bothering me when I originally got this unit test > module to compile, namely, that the tests themselves are only about a single > class, and yet it takes eighteen extra non-header source files just to get the > tests to link correctly. A few extra files wouldn't be all that surprising, but > so many is indicative of larger issues in the structure of the code. > > > > I'm fine with this as a temporary thing, but we should at least file a > > follow-up issue for removing the duplication here. > > We certainly could, but I don't know what that issue would be, exactly. We could > build a static library to house code common to the plugin and its tests, for > example, or we could simple put common build elements in GYP variables. Nothing > was obviously the right thing when I set this up, and that hasn't changed since. I'd generally find a static library cleaner, but I'm not sure if that's going to get rid of all the duplication here. Either way, the follow-up issue doesn't need to be too specific, it can just say that we want to remove duplication between the application and test targets.
LGTM. Beside comments in code I would like to say that in most cases it's better to use EXPECT_* instead of ASSERT_* but in these particular cases it's not important. 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) I think we need some well known abbreviation like CRIM = "const reference is missed". http://codereview.adblockplus.org/6267413888761856/diff/5700735861784576/test... test/plugin/UserSettingsTest.cpp:40: void SingleMethodNameNotFound(std::wstring name) CRIM
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. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
