Issue #1163 - add unit tests for CPluginUserSettings

test/plugin/UserSettingsTest.cpp

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2014 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include <gtest/gtest.h>
 
 #include <OAIdl.h>
 #include "../../src/plugin/PluginUserSettings.h"
 
 //----------------------------------
 // GetIDsOfNames
 //----------------------------------
 
 namespace
 {
-  void single_method_name_found(std::wstring name, DISPID expected_id)
+  void SingleMethodNameFound(std::wstring name, DISPID expected_id)

[sergei, 2014/11/03 14:41:36]

I think we need some well known abbreviation like CRIM = "const reference is
missed".

[Felix Dahlke, 2014/11/03 16:05:18]

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.

   {
     CPluginUserSettings x;
-    wchar_t *names[1];
-    names[0] = const_cast<wchar_t *>(name.c_str());
+    wchar_t* names[] = {const_cast<wchar_t*>(name.c_str())};
     DISPID ids[1];
     HRESULT h = x.GetIDsOfNames(IID_NULL, names, 1, 0, ids);
     ASSERT_EQ(S_OK, h);
-    DISPID id=ids[0];
+    DISPID id = ids[0];
     ASSERT_EQ(expected_id, id);
   }
 
-  void single_method_name_notfound(std::wstring name)
+  void SingleMethodNameNotFound(std::wstring name)

[sergei, 2014/11/03 14:41:36]

CRIM

   {
     CPluginUserSettings x;
-    wchar_t *names[1];
-    names[0] = const_cast<wchar_t *>(name.c_str());
+    wchar_t* names[] = {const_cast<wchar_t*>(name.c_str())};
     DISPID ids[1];
     HRESULT h = x.GetIDsOfNames(IID_NULL, names, 1, 0, ids);
     ASSERT_NE(S_OK, h);
     // The old version returns a nonstandard error code.
     if (h == DISP_E_MEMBERNOTFOUND)
     {
       return;
     }
     EXPECT_EQ(DISP_E_UNKNOWNNAME, h);
   }
 }
 
-TEST(CPluginUserSettings_GetIDsOfNames_Test, all_defined_found)
+TEST(CPluginUserSettingsGetIDsOfNames, AllDefinedMethodsMustBeFound)
 {
   CPluginUserSettings x;
-  single_method_name_found(L"GetMessage", 0);
-  single_method_name_found(L"GetLanguageCount", 1);
-  single_method_name_found(L"GetLanguageByIndex", 2);
-  single_method_name_found(L"GetLanguageTitleByIndex", 3);
-  single_method_name_found(L"SetLanguage", 4);
-  single_method_name_found(L"GetLanguage", 5);
-  single_method_name_found(L"GetWhitelistDomains", 6);
-  single_method_name_found(L"AddWhitelistDomain", 7);
-  single_method_name_found(L"RemoveWhitelistDomain", 8);
-  single_method_name_found(L"GetAppLocale", 9);
-  single_method_name_found(L"GetDocumentationLink", 10);
-  single_method_name_found(L"IsAcceptableAdsEnabled", 11);
-  single_method_name_found(L"SetAcceptableAdsEnabled", 12);
+  SingleMethodNameFound(L"GetMessage", 0);
+  SingleMethodNameFound(L"GetLanguageCount", 1);
+  SingleMethodNameFound(L"GetLanguageByIndex", 2);
+  SingleMethodNameFound(L"GetLanguageTitleByIndex", 3);
+  SingleMethodNameFound(L"SetLanguage", 4);
+  SingleMethodNameFound(L"GetLanguage", 5);
+  SingleMethodNameFound(L"GetWhitelistDomains", 6);
+  SingleMethodNameFound(L"AddWhitelistDomain", 7);
+  SingleMethodNameFound(L"RemoveWhitelistDomain", 8);
+  SingleMethodNameFound(L"GetAppLocale", 9);
+  SingleMethodNameFound(L"GetDocumentationLink", 10);
+  SingleMethodNameFound(L"IsAcceptableAdsEnabled", 11);
+  SingleMethodNameFound(L"SetAcceptableAdsEnabled", 12);
 }
 
-TEST(CPluginUserSettings_GetIDsOfNames_Test, undefined_not_found)
+TEST(CPluginUserSettingsGetIDsOfNames, UndefinedMethodsMustNotBeFound)
 {
-  single_method_name_notfound(L"");
-  single_method_name_notfound(L"clearly unknown");
-  single_method_name_notfound(L"GETMESSAGE");
+  SingleMethodNameNotFound(L"");
+  SingleMethodNameNotFound(L"clearly unknown");
+  SingleMethodNameNotFound(L"GETMESSAGE");
 }
 
 //----------------------------------
 // Invoke
 //----------------------------------
 
 namespace
 {
-  void invoke_invalid_dispatch_id(DISPID id)

[sergei, 2014/08/14 11:37:26]

`id` is not used.

[Eric, 2014/08/14 14:29:53]

Oops. Forgot to replace the -1 below when I refactored from the first version.

+  void InvokeInvalidDispatchId(DISPID id)
   {
     CPluginUserSettings x;
     DISPPARAMS params;

[sergei, 2014/08/14 11:37:26]

DISPPARAMS params = {} looks better, but OK here.

[Eric, 2014/08/14 14:29:53]

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.

     params.rgvarg = nullptr;
     params.rgdispidNamedArgs = nullptr;
     params.cArgs = 0;
     params.cNamedArgs = 0;
     EXCEPINFO ex;

[sergei, 2014/08/14 11:37:26]

I would also initialize it.

-    HRESULT h = x.Invoke(-1, IID_NULL, 0, 0, &params, nullptr, &ex, nullptr);

[sergei, 2014/08/14 11:37:26]

The first arg should be `id`.

[sergei, 2014/08/14 11:37:26]

Why is the flag 0? As I see we expose only methods, so we should use and check
DISPATCH_METHOD.

[Eric, 2014/08/14 14:29:53]

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.

+    HRESULT h = x.Invoke(id, IID_NULL, 0, DISPATCH_METHOD, &params, nullptr, &ex, nullptr);
     ASSERT_NE(S_OK, h);
     // The old version returns a nonstandard error code.
     if (h == DISP_E_BADINDEX)
     {
       return;
     }
     EXPECT_EQ(DISP_E_MEMBERNOTFOUND, h);
   }
 }
 
-TEST(CPluginUserSettings_Invoke_Test, invalid_dispatch_id_underflow)

[sergei, 2014/08/14 11:37:26]

I prefer more descriptive test names.

[Eric, 2014/08/14 14:29:53]

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.

+/**
+ * Verify that a negative Dispatch ID returns the proper error code.
+ */
+TEST(CPluginUserSettingsInvoke, InvalidDispatchIdShouldUnderflow)
 {
-  invoke_invalid_dispatch_id(-1);
+  InvokeInvalidDispatchId(-1);
 }
 
-TEST(CPluginUserSettings_Invoke_Test, invalid_dispatch_id_overflow)
+/**
+ * Verify that a positive Dispatch ID that's too large returns the proper error code.
+ */
+TEST(CPluginUserSettingsInvoke, InvalidDispatchIdShouldOverflow)
 {
-  invoke_invalid_dispatch_id(13);
-}
+  InvokeInvalidDispatchId(13);
+}