Issue #41 - Bring method of determining IE version up to date

test/RegistryTest.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 "../src/shared/Registry.h"
 #include "../src/shared/IE_Version.h"
 
 using namespace AdblockPlus;
 
 //----------------------------------
 // Registry_Key
 //----------------------------------
-
-class Registry_Test

[sergei, 2014/07/28 11:46:27]

It's not needed, remove it.

[Eric, 2014/07/29 12:42:25]

Explicit class declarations are required for command-line option --gtest_filter
works well.

[sergei, 2014/07/29 14:45:56]

This class is even not used, because tests are not feature based, they are
TEST(), not TEST_F(). Command line works well with for the test name as well.

[Eric, 2014/07/29 15:17:22]

Go read the documentation about how --gtest_filter works. I use extensively.

[sergei, 2014/07/30 10:42:04]

I've checked the documentation and tried it and it does work. `Registry_Test` is
not needed and this class is even not used by gtest.

[Eric, 2014/07/30 13:02:04]

OK. Tested on my end.

Where in the googletest documentation does it state that you don't need to
declare the test class? I've missed it, but that documentation isn't the best
organized, either.

-  : public ::testing::Test
+TEST(RegistryTest, simple_0)
 {
-};
-
-TEST(Registry_Test, simple_0)

[sergei, 2014/07/28 11:46:27]

What does `simple_0` mean? Call it like `RegistryKeyTest,
ctr_should_successfully_open_CLSID`

-{
-  ASSERT_NO_THROW({ auto r = Registry_Key(HKEY_CLASSES_ROOT, L"CLSID"); });
+  ASSERT_NO_THROW({ auto r = RegistryKey(HKEY_CLASSES_ROOT, L"CLSID"); });
 }
 
-TEST(Registry_Test, constructor_illegal_argument_0)

[sergei, 2014/07/28 11:46:27]

`ctr_should_throw_exception_if_key_is_empty`

+TEST(RegistryTest, constructor_illegal_argument_0)
 {
-  ASSERT_ANY_THROW({ auto r1 = Registry_Key(HKEY_CLASSES_ROOT, L""); });
+  ASSERT_ANY_THROW({ auto r1 = RegistryKey(HKEY_CLASSES_ROOT, L""); });
 }
 
-TEST(Registry_Test, value_notfound_0)

[sergei, 2014/07/28 11:46:27]

StringValue_should_throw_if_value_not_found

+TEST(RegistryTest, value_notfound_0)
 {
-  auto r1 = Registry_Key(HKEY_CLASSES_ROOT, L"CLSID");
+  auto r1 = RegistryKey(HKEY_CLASSES_ROOT, L"CLSID");
   ASSERT_ANY_THROW({ r1.value_wstring(L"nonexistent"); });
 }
 
 //----------------------------------

[sergei, 2014/07/28 11:46:27]

Move it into another cpp file

[Eric, 2014/07/29 12:42:25]

At least for now, that's overkill. 

[sergei, 2014/07/29 14:45:56]

It's not an overkill
- it will be difficult to locate the tests, confusing and and will cause
questions when we need it.
- it accumulates the technical debt.

 // IE_Version

[sergei, 2015/01/06 13:39:11]

The tests below should be in IE_versionTest.cpp

[Eric, 2015/01/06 17:10:34]

OK. I'll do it in a later change. We need to move the registry and IE version
classes into a library shared by both the plugin/engine and the installer. We
don't currently have such a library in our build system.

 //----------------------------------
-
-class IE_Version_Test

[sergei, 2014/07/28 11:46:27]

It's not needed

-  : public ::testing::Test
-{
-};
-
+/*
+ * Exact version tests enabled by default.
+ * If they are disabled, gtext will report 2 disabled tests.
+ */
+#if !defined(DISABLE_EXACT_TESTS)
+#define exact(x) exact_##x

[sergei, 2015/01/06 13:39:11]

What is the aim of it?

I would remove it.

[Eric, 2015/01/06 17:10:34]

Ordinarily unit tests should always run independent of the system that they're
installed on. Unfortunately that can't work for testing the installed version of
IE, since the relevant data is held in system-global variable in the registry.

These definitions allow a working copy to adapt these unit tests to local
conditions.

+#else
+#define exact(x) DISABLED_exact_##x
+#endif
+/*
+ * Define a default version as the current version of IE in general release.
+ * Predefine on the command line to compile tests for non-standard development environments.
+ */
 #ifndef INSTALLED_IE_VERSION
 #define INSTALLED_IE_VERSION 11
 #endif
 
-#if INSTALLED_IE_VERSION == 11
-#define perform_version_test true
-#define expected_version_string L"11."

[sergei, 2014/07/28 11:46:27]

Why do we need the point at the end?

[Eric, 2014/07/29 12:42:25]

It's the last character of the longest initial substring guaranteed to be shared
by all version strings.

[sergei, 2014/07/29 14:45:56]

Still I have not get your point. "11" or "10" or "8" or "453" works fine.

-#else
-#define perform_version_test false
-#endif
-
 TEST(IE_Version_Test, sanity_string)
 {
-  std::wstring version = AdblockPlus::IE::installed_version_string();
-  ASSERT_NE(version, L"");
-  // Replace with local version prefix as appropriate
-  if (perform_version_test)
-  {
-    ASSERT_EQ(0, version.compare(0, 3, expected_version_string));
-  }
+  std::wstring version = AdblockPlus::IE::InstalledVersionString();
+  ASSERT_FALSE(version.length() == 0); // separate test for default value
+  ASSERT_TRUE(version.length() >= 2);
 }
 
 TEST(IE_Version_Test, sanity_major)
 {
-  int version = AdblockPlus::IE::installed_major_version();
-  ASSERT_NE(version, 0);
-  // Replace with local major version
-  if (perform_version_test)
-  {
-    ASSERT_EQ(version, INSTALLED_IE_VERSION);

[Oleksandr, 2014/07/27 22:13:30]

What if we just tested here for a range of values instead? Like:
ASSERT_LT(version, 12)
ASSERT_GE(version, 6)

This will be a decent check on it's own, IMHO. It will eliminate that explicit
define above and it will also automatically remind us to test the IE version
routine when IE 12 goes public. Same for the version string test.

[sergei, 2014/07/28 11:46:27]

Agree about the range, get rid of the hacks `perform_version_test`

[Eric, 2014/07/28 11:48:43]

The problem is that we shouldn't make many assumptions about the installation
state of this test suite. For developers, sure, we can assume they have the
current version. On the other hand, if/when we ever get to automated testing on
older platforms, say XP, we may get stuck with an older version of IE. 

I thought about a number of ways of dealing with this problem. The previous
version commented out this check, performing just the sanity check. I settled on
putting the version number in a preprocessor definition, allowing it to be
passed on the command line. This will support automation at a future point and
also provide a decent default for developers.

[sergei, 2014/07/29 14:45:56]

What does this test tests? `sanity_major` says nothing. If it's intended to test
the extraction of the major version from the version string, then it's better to
have the function `GetMajorNumber` is I've mentioned above and test it.

-  }
+  int version = AdblockPlus::IE::InstalledMajorVersion();
+  ASSERT_NE(version, 0); // separate test for default value
+  ASSERT_GE(version, 6);
+  ASSERT_LE(version, 12);
+  EXPECT_NE(version, 12); // This check will point out when the test needs updating.
 }
+
+TEST(IE_Version_Test, exact(string))
+{
+    std::wstring version = AdblockPlus::IE::InstalledVersionString();
+    std::wstring expected = std::to_wstring(INSTALLED_IE_VERSION) + L".";
+    ASSERT_EQ(expected, version.substr(0, expected.length()));
+}
+
+TEST(IE_Version_Test, exact(major))
+{
+  int version = AdblockPlus::IE::InstalledMajorVersion();
+  ASSERT_EQ(version, INSTALLED_IE_VERSION);
+}