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(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"); });
+}
+
+TEST(Registry_Test, constructor_illegal_argument_0)

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

`ctr_should_throw_exception_if_key_is_empty`

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

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

StringValue_should_throw_if_value_not_found

+{
+  auto r1 = Registry_Key(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
+//----------------------------------
+
+class IE_Version_Test

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

It's not needed

+  : public ::testing::Test
+{
+};
+
+#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));
+  }
+}
+
+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.

+  }
+}