Added test files for SystemInfo.m and OmahaXMLRequest.m.

chrome/installer/mac/app/testing/SystemInfo_test.mm

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+#import "chrome/installer/mac/app/SystemInfo.h"
+
+namespace {
+
+class SystemInfoTest : public ::testing::Test {

[Mark Mentovai, 2016/07/22 15:42:38]

We shouldn’t have a test class here or any TEST_F. Instead…

[Anna Zeng, 2016/07/22 16:58:03]

So I know that fixtures are used when each test requires something -- is the
main reason we don't use fixtures because the tests have become so concise?

[Mark Mentovai, 2016/07/22 18:03:24]

Anna Zeng wrote:
Yes! If there’s a lot of common set-up and tear-down stuff, a fixture makes
sense. If you need an object that subclasses something else to write a test, a
fixture may also make sense (but may also not). But when we can do more (or at
least just as much) with less code, a fixture is unnecessary.

The gtest primer
(https://github.com/google/googletest/blob/master/googletest/docs/Primer.md)
says:
and then explains a bit more in the “Test Fixtures: Using the Same Data
Configuration for Multiple Tests” section.

+ protected:
+  SystemInfoTest() : Test() {
+    arch = [SystemInfo getArch];
+    os_version = [SystemInfo getOSVersion];
+  }
+  NSString* arch;
+  NSString* os_version;
+};
+
+TEST_F(SystemInfoTest, GetArchReturnsExpectedString) {
+  EXPECT_TRUE([arch isEqualToString:@"i486"] ||

[Mark Mentovai, 2016/07/22 15:42:38]

This should just be TEST(SystemInfoTest, GetArchReturnsExpectedString) {
  NSString* arch = [SystemInfo getArch];
}

Because there’s no reason for a class and an object and all of that extra goop
when we can just get what we need from right here. Less code, easier to read,
less distracting stuff to do at runtime. For example, we don’t care about the OS
version for this test, so why did we bother fetching it in the SystemInfoTest
class?

[Anna Zeng, 2016/07/22 16:58:03]

Done.

+              [arch isEqualToString:@"x86_64h"]);
+}
+
+TEST_F(SystemInfoTest, GetOSVersionMatchesRegexFormat) {
+  NSRegularExpression* regex = [NSRegularExpression

[Mark Mentovai, 2016/07/22 15:42:38]

Similar here, just get the OS version right here in this TEST() (not TEST_F()).

[Anna Zeng, 2016/07/22 16:58:02]

Done.

+      regularExpressionWithPattern:@"^10\\.[0-9]+\\.[0-9]+$"

[Mark Mentovai, 2016/07/22 15:42:38]

We can make the regex even tighter:

"^10\\.(0|[1-9][0-9]*)\\.(0|[1-9][0-9]*)$"

To avoid matching 10.11.01, but to allow 10.11.0 to match.

[Anna Zeng, 2016/07/22 16:58:02]

Oh wow, good point! Done.

+                           options:0
+                             error:nil];
+  NSUInteger matches =
+      [regex numberOfMatchesInString:os_version
+                             options:0
+                               range:NSMakeRange(0, os_version.length)];
+  EXPECT_EQ((int)matches, 1);

[Mark Mentovai, 2016/07/22 15:42:38]

Rather than a cast to int, you can say 1u, which means “1 as unsigned”. This is
a quirk of gtest, because normally signed-unsigned comparisons are tolerable and
follow defined rules. With gtest macros, that doesn’t happen in exactly the same
way.

Also, gtest macros annoyingly put the “expected” side on the left, so this
should be:

EXPECT_EQ(1u, matches);

[Anna Zeng, 2016/07/22 16:58:02]

Thanks for the tip! Looking closer at NSInteger/NSUInteger, it seems they're
typedefs of either long or unsigned long. So just to check my understanding: it
is a safe comparison to compare NSIntegers with integers, and NSUIntegers with
uints, because the ints and uints would automatically be promoted to long/ulongs
when compared to NSInteger/NSUInteger. Is that true?

[Mark Mentovai, 2016/07/22 18:03:24]

Anna Zeng wrote:
Right. NSInteger and NSUInteger are defined in <objc/NSObjCRuntime.h> (for
__LP64__, which is what we always are now):

typedef long NSInteger;
typedef unsigned long NSUInteger;

A comparison between long and int always promotes the into to long and then does
the comparison. Likewise, a comparison between unsigned long and unsigned int
always promotes the unsigned into to unsigned long and then does the comparison.
So you don’t need to get super-picky about whether you say the constant is 1u
(which is an unsigned int) or 1ul (which would be an unsigned long), because if
you’re comparing it against an unsigned long, it’ll always be promoted.

Why not go nuts and write 1ul instead of 1u? Well, it’s not necessary. And in
some environments, NSInteger is actually int instead of long, and NSUInteger is
actually unsigned int instead of unsigned long, so going crazy doesn’t really
buy you much.

Details on integer promotion:
http://en.cppreference.com/w/cpp/language/implicit_conversion#Integral_promotion

Now, there are also rules on integer conversion
(http://en.cppreference.com/w/cpp/language/implicit_conversion#Integral_conver...)
which are non-promotion conversions between unrelated integer types. This is
what you get when you’re comparing signed with unsigned, and in this case, it’s
what you get when you have an NSUInteger aka long (matches) being compared
against an int (1 without any u). In this case, 1 (int) can be converted to 1u
(unsigned int) if that’s what’s called for, or even to 1ul (unsigned long) if
that’s what’s called for, without any problem.

So what was wrong with EXPECT_EQ(1, matches)? It basically just translates into
a conditional like |if (1 == matches)|, so why don’t the conversion rules kick
in and convert 1 (int) into something more compatible with matches (unsigned
long)?

The problem is -Wsign-compare, which turns comparisons like this into warnings.
It doesn’t look like a problem when the signed int is a constant like 1, because
you see 1 and you know that it can be represented in any size unsigned format
too. But on the other hand, it’s bogus to write a comparison like |if
(some_other_int == matches)|, because you don’t know the value of
some_other_int, and if it is in fact negative, the comparison might not
(probably won’t) test what you intend. More importantly, when it’s a variable
like that, the compiler doesn’t know it either.

And in fact, this is where things get interesting, and why I said “gtest is
weird.” You actually DON’T get a warning from -Wsign-compare in most cases where
you write a constant right in the expression itself. So if we had written this
as:

  EXPECT_TRUE(matches == 1);

the compiler wouldn’t have had anything to say about it, and neither would Elly
or I. Why? matches is still unsigned long, and 1 is still an int, and
-Wsign-compare is still in effect, so why didn’t it complain? Because the
compiler’s not a complete and total dolt. It sees the constant, it knows its
value right there as used in the expression, and it knows that 1 can safely be
converted to an unsigned int (or unsigned long, even) without losing data and
without maybe (probably) making the comparison test something that you didn’t
intend.

So then what’s wrong with this?

  EXPECT_EQ(1, matches);

Let’s look at how EXPECT_EQ is defined in "gtest/gtest.h":

#define EXPECT_EQ(expected, actual) \
  EXPECT_PRED_FORMAT2(::testing::internal:: \
                      EqHelper<GTEST_IS_NULL_LITERAL_(expected)>::Compare, \
                      expected, actual)

It looks like the comparison happens in
testing::internal::EqHelper<>::Compare<>(). And that’s the problem! By the time
your constant int “1” makes it into that function, it’s no longer a constant.
It’s just some int whose value the compiler doesn’t know. Notably, it doesn’t
know if it’s negative, so it can’t say for sure that it’s safe to compare it
against “matches”, the unsigned long.

In reality, because all of the macros and templates involved are inlined, the
compiler actually does see everything at once, and if it were sufficiently
advanced, it could have seen the constant “1” flow all the way from
SystemInfo_test.mm into the dark bowels of gtest where the comparison happens,
and decided to cut you some slack. But today’s compilers don’t do that. Once you
stick your constant into a variable (which happens implicitly when you make the
testing::internal::EqHelper<>::Compare<>() call), all hope is lost. It’s no
longer “1”, even for this incantation of EXPECT_EQ() where you and I and Ivan
can all agree that it’s 1. It’s just some random anonymous int, and
-Wsign-compare kicks in to keep you from hurting yourself and others.

So one more example to drive it all home. Look at this inane function:

1  bool F() {
2    int x = 1;
3    unsigned int y = x;
4    return x == y;
5  }

Line 3 is valid and not in dispute because the error’s about -Wsign-compare, not
-Wsign-assign. And with even modest optimization enabled, the compiler’s smart
enough to know that this always returns true, and won’t even bother making the
CPU do anything: it just turns the whole thing into “return true;” and calls it
a day. But even in that case, line 4 will still trigger the warning. x is an
int, it could be anything, it could even be (gasp) negative!

So every Chrome developer learns early in their career that this pattern of
EXPECT()ing things with gtest requires a quirky “u” if the thing you’re
EXPECT()ing is unsigned. Sure, we’re humoring the compiler.

Except on Windows. Someone found a way to suppress the warning that MSVC
generates here. That’s line 1391 here:
https://github.com/google/googletest/blob/master/googletest/include/gtest/gte....
And the warning can in fact be ignored right there for clang and gcc too. So why
hasn’t anyone done this? I don’t know! Probably someone just needs to go in and
do it, and then we can remove this quirky workaround from our collective mind.

[Anna Zeng, 2016/07/22 18:38:32]

This is an amazing book Mark! I highly recommend this for publishing on your
online column. :D

+}
+
+}  // namespace