Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2137743002:
Added test files for SystemInfo.m and OmahaXMLRequest.m. (Closed)
DescriptionAdded test file for SystemInfo.m, on top of the initial commit for Chrome meta installer.
Just look at BUILD.gn and SystemInfo_test.mm, everything else is consistent with CL 2094583004 (Initial commit for Chrome meta installer https://codereview.chromium.org/2094583004/)
BUG=
patch from issue 2106523002 at patchset 60001 (http://crrev.com/2106523002#ps60001)
Committed: https://crrev.com/10fd066ee3eae1695902044db958a85394ad3fe8
Cr-Commit-Position: refs/heads/master@{#407573}
Patch Set 1 #Patch Set 2 : Completed both test files #
Total comments: 46
Patch Set 3 : Added comments & made progress on roughly half of Mark's comments #
Total comments: 4
Patch Set 4 : Addressed all comments except Mark's comment on getArch #Patch Set 5 : Transitioned fully to super-concise tests; ready for more comments! #
Total comments: 18
Patch Set 6 : Eliminated fixtures #
Total comments: 2
Patch Set 7 : Last change before LGTM! #
Total comments: 1
Patch Set 8 : Fix for the trybot #Patch Set 9 : Attempt two for the trybots #Patch Set 10 : Added some copyright content to please the trybot lords #
Messages
Total messages: 58 (22 generated)
Description was changed from ========== Added test file for SystemInfo.m Added assert resolved some merge conflicts not merged Add a few more changes Merged ivan addressed some more comments Used `git cl format` to adjust style merged with ivan Fixed patch cl blah pre-cl warnings taken care of Patch 2106523002 from Ivan Reconcile differences from rebasing Eliminated request.* Another config file change for warnings in pre-cl Edited another BUILD file Resolved pre-cl warnings Moved dependencies away from root BUILD file. Merge conflicts solved, fixed up comments Eliminated pre-cl warnings Implemented Network Communication Fixed the last problem with OmahaCommunication Re-did a BUILD change Cleaned up code, added TODO about asynchronousity Fixed the payload static problem using the keyword `retain`. Broke methods up further and added comments Breaking long blcoks of code up Added header gaurds to files. Added copyright headers and removed extra whitespace. Initial commit for Chrome meta installer. BUG= patch from issue 2106523002 at patchset 80001 (http://crrev.com/2106523002#ps80001) Fixed pre-cl warnings Removed root stuff from root BUILD.gn Fully migrated to new design organization Broke methods up further and added comments Breaking long blcoks of code up Added header gaurds to files. Added copyright headers and removed extra whitespace. Initial commit for Chrome meta installer. BUG= patch from issue 2106523002 at patchset 60001 (http://crrev.com/2106523002#ps60001) ========== to ========== Added test file for SystemInfo.m, on top of the initial commit for Chrome meta installer. Just look at BUILD.gn and SystemInfo_test.mm BUG= patch from issue 2106523002 at patchset 60001 (http://crrev.com/2106523002#ps60001) ==========
zengster@google.com changed reviewers: + ellyjones@chromium.org, ivanhernandez@google.com, mark@chromium.org
Description was changed from ========== Added test file for SystemInfo.m, on top of the initial commit for Chrome meta installer. Just look at BUILD.gn and SystemInfo_test.mm BUG= patch from issue 2106523002 at patchset 60001 (http://crrev.com/2106523002#ps60001) ========== to ========== Added test file for SystemInfo.m, on top of the initial commit for Chrome meta installer. Just look at BUILD.gn and SystemInfo_test.mm, everything else is consistent with CL 2094583004 (Initial commit for Chrome meta installer https://codereview.chromium.org/2094583004/) BUG= patch from issue 2106523002 at patchset 60001 (http://crrev.com/2106523002#ps60001) ==========
The code review duplicates all of the original change, so I’ll wait until you’ve rebased on the soon-to-be-landed initial commit instead of sorting through this.
Ready for review!
https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/BUILD.gn (right): https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:5: static_library("project") { We should come up with better names, since these are kind of globally namespaced within all of Chrome. Prefix things with mac_installer? https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:27: executable("downloader_test") { Probably just mac_installer_test, and we can have all of our tests run from a single executable. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:30: "//testing/gtest/src/gtest_main.cc", I think you want to depend on the gtest_main source set, which already compiles this, rather than compiling another copy in this target. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm (right): https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:5: #include "../OmahaXMLRequest.h" #import this one. Also, we should probably move to more absolute paths for #includes. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:16: virtual ~OmahaXMLRequestTest() { [xmlBody release]; } Here’s where scoped_nsobject would be handy: you won’t need to write your own releases in destructors if you’re using scoped_nsobject<NSXMLDocument> xmlBody_; https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:18: NSXMLDocument* xmlBody; Remember to use trailing underscores for your member variables. Also, use C++-style naming, which is underscores separating words: xml_body_. https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:18: NSXMLDocument* xmlBody; This should be in a private: section. https://google.github.io/styleguide/cppguide.html#Access_Control https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:19: }; The private: section should also end with DISALLOW_COPY_AND_ASSIGN. https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:22: EXPECT_NE(xmlBody, nil); EXPECT_TRUE(xml_body_) is fine. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:22: EXPECT_NE(xmlBody, nil); Not sure that you need this independent test case. createReturnsValidXML effectively tests this too. If you want to be explicit about it, you can stick EXPECT_TRUE(xml_body_) in as the first line of createReturnsValidXML. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:25: TEST_F(OmahaXMLRequestTest, createReturnsValidXML) { Test case names should begin with capital letters. This applies in this file and in the system info test. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:37: Do you want to parse the XML a bit more to make sure that it’s got the things that we intended to set in it? https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/SystemInfo_test.mm (right): https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:5: #include "../SystemInfo.h" #import https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:10: class SystemInfoTest : public ::testing::Test {}; Don’t need this at all. You haven’t used it, since you’ve only got TEST, not TEST_F. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:13: EXPECT_NE([SystemInfo getArch], nil); EXPECT_TRUE is fine. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:21: EXPECT_GE((int)[[SystemInfo getArch] length], 3); You’ve got three tests where I think that one will do: - getArchReturnsAtLeastThreeCharacters implies that the arch was a string, so getArchReturnsAString isn’t necessary - getArchReturnsAString implies that the return was non-nil, so getArchReturnsAString isn’t necessary Actually, I think we should nail the test down a bit harder. We know we only want to see x86_64h or i486 here (ugh). So let’s just test that this returns one of those things. We don’t need class tests or length tests or non-nil checks. EXPECT_TRUE([arch isEqualToString:@"i486"] || [arch isEqualToString:@"x86_64h"]);. If you’re compelled to, you can stick this in an #if defined(__x86_64__) and #error on other architectures, the same as in the .mm file. So you’ll be writing less code but actually testing more. Efficiency! https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:43: TEST(SystemInfoTest, getOSVersionReturnsNumbers) { Similar here. Let’s just make sure that the format is "10.digits.digits". NSRegularExpression might be the easiest way to do this, actually. Again, probably only this needs one TEST to cover getOSVersion. If you want to get fancy, make sure that the runs of digits don’t begin with zero unless there’s only a single digit in there (as in 10.11.0).
https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm (right): https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:18: NSXMLDocument* xmlBody; On 2016/07/15 21:56:09, Mark Mentovai wrote: > This should be in a private: section. > https://google.github.io/styleguide/cppguide.html#Access_Control From the above link: " For technical reasons, we allow data members of a test fixture class to be protected when using Google Test)."
https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/requestCheck.dtd (right): https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/requestCheck.dtd:1: <!ELEMENT request (os, app)> can you please comment this file heavily? I (and I think most of the team) can't read XML DTDs.
On 2016/07/19 18:45:59, Elly Jones wrote: > https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... > File chrome/installer/mac/app/testing/requestCheck.dtd (right): > > https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... > chrome/installer/mac/app/testing/requestCheck.dtd:1: <!ELEMENT request (os, > app)> > can you please comment this file heavily? I (and I think most of the team) can't > read XML DTDs. Added comments in aforementioned file! You are welcome to take a gander.
https://codereview.chromium.org/2137743002/diff/40001/chrome/installer/mac/ap... File chrome/installer/mac/app/BUILD.gn (right): https://codereview.chromium.org/2137743002/diff/40001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:27: executable("mac_installer_test") { OK! Got a better answer here. This should use the test() target type instead of executable(). That should take care of the gtest dependencies for you. You’ll find examples in .gn files elsewhere in the codebase. https://codereview.chromium.org/2137743002/diff/40001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:36: "//testing/gtest:gtest_main", And the answer on this one is to use //base/test:run_all_unittests instead of gtest_main. (You can also remove the commented-out line 30.)
Ready for more comments if any! Mark, I've got a small question for you tomorrow morning! https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm (right): https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:19: }; On 2016/07/15 21:56:08, Mark Mentovai wrote: > The private: section should also end with DISALLOW_COPY_AND_ASSIGN. > https://google.github.io/styleguide/cppguide.html#Declaration_Order Since there is no private: section and this is test code, I will not include this macro. Robert also mentioned that he seldom includes it as well. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:37: On 2016/07/15 21:56:08, Mark Mentovai wrote: > Do you want to parse the XML a bit more to make sure that it’s got the things > that we intended to set in it? I'm able to currently guarantee lack of whitespace in attribute values and for some, I can specify exactly what string the value should be. I can't use regular expressions within the DTD files though. Would it be a good idea to include regex to check the values of the attributes? https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/SystemInfo_test.mm (right): https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:21: EXPECT_GE((int)[[SystemInfo getArch] length], 3); On 2016/07/15 21:56:09, Mark Mentovai wrote: > You’ve got three tests where I think that one will do: > > - getArchReturnsAtLeastThreeCharacters implies that the arch was a string, so > getArchReturnsAString isn’t necessary > - getArchReturnsAString implies that the return was non-nil, so > getArchReturnsAString isn’t necessary > > Actually, I think we should nail the test down a bit harder. We know we only > want to see x86_64h or i486 here (ugh). So let’s just test that this returns one > of those things. We don’t need class tests or length tests or non-nil checks. > EXPECT_TRUE([arch isEqualToString:@"i486"] || [arch > isEqualToString:@"x86_64h"]);. If you’re compelled to, you can stick this in an > #if defined(__x86_64__) and #error on other architectures, the same as in the > .mm file. > > So you’ll be writing less code but actually testing more. Efficiency! So... you mean that we want the code to work ONLY on i486 or x86_64, and to throw an error for all other architectures? Are... you sure?
https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/SystemInfo_test.mm (right): https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:21: EXPECT_GE((int)[[SystemInfo getArch] length], 3); On 2016/07/22 01:45:37, Anna Zeng wrote: > On 2016/07/15 21:56:09, Mark Mentovai wrote: > > You’ve got three tests where I think that one will do: > > > > - getArchReturnsAtLeastThreeCharacters implies that the arch was a string, so > > getArchReturnsAString isn’t necessary > > - getArchReturnsAString implies that the return was non-nil, so > > getArchReturnsAString isn’t necessary > > > > Actually, I think we should nail the test down a bit harder. We know we only > > want to see x86_64h or i486 here (ugh). So let’s just test that this returns > one > > of those things. We don’t need class tests or length tests or non-nil checks. > > EXPECT_TRUE([arch isEqualToString:@"i486"] || [arch > > isEqualToString:@"x86_64h"]);. If you’re compelled to, you can stick this in > an > > #if defined(__x86_64__) and #error on other architectures, the same as in the > > .mm file. > > > > So you’ll be writing less code but actually testing more. Efficiency! > > So... you mean that we want the code to work ONLY on i486 or x86_64, and to > throw an error for all other architectures? Are... you sure? Remember the macro that we stuck at the top of SystemInfo.h? If we ever did find ourselves building for a non-__x86_64__ system, the .m file would #error too, and if we fixed that, we’d also want to fix this test. We know that i486 and x86_64h are the only things that the API we’re using can return now. We also know that it’s quirky, because i486 is kind of a weird thing to return on a 64-bit system to begin with. So we’d just be duplicating that in our test, basically saying “we know that these are the only two things we’ve ever seen that weird call return, and if it ever starts returning something else, we probably want to verify that it’s correct.”
Thanks Mark for your help! I'm ready for more perspectives, everyone!
https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm (right): https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:22: EXPECT_NE(xmlBody, nil); I wrote: > Not sure that you need this independent test case. createReturnsValidXML > effectively tests this too. If you want to be explicit about it, you can stick > EXPECT_TRUE(xml_body_) in as the first line of createReturnsValidXML. This comment from last time was unaddressed. https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm (right): https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:21: EXPECT_TRUE(xml_body_); (this is what I was talking about—this was unaddressed from the last round of feedback.) https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:25: NSString* requestDTDLocation = When you move EXPECT_TRUE(xml_body_) here, you’ll notice that you have this one test class that does one thing ([OmahaXMLRequest createXMLRequestBody]) for one caller only (OmahaXMLRequestTest.CreateReturnsValidXML). Seems kinda spread out and not all that necessary! At that point, you don’t need the test class at all! You can just write this as a TEST() (not TEST_F()) that starts out as: base::scoped_nsobject<NSXMLDocument> xml_body( [OmahaXMLRequest createXMLRequestBody]); ASSERT_TRUE(xml_body); NSString* requestDTDLocation = … Note the use of ASSERT_TRUE instead of EXPECT_TRUE now. That’s because if xml_body is nil, it’s pointless to continue the test. ASSERT_TRUE will stop it dead in its tracks right there. If we allowed it to continue, surely we’d hit another error at the EXPECT_TRUE at the bottom of the function, because sending a message to the nil xml_body would return nil, which would fail the EXPECT_TRUE. But that second failure would just mask the first, real one, which is that xml_body was nil to begin with. https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/SystemInfo_test.mm (right): https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:11: class SystemInfoTest : public ::testing::Test { We shouldn’t have a test class here or any TEST_F. Instead… https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:22: EXPECT_TRUE([arch isEqualToString:@"i486"] || 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? https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:27: NSRegularExpression* regex = [NSRegularExpression Similar here, just get the OS version right here in this TEST() (not TEST_F()). https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:28: regularExpressionWithPattern:@"^10\\.[0-9]+\\.[0-9]+$" 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. https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:35: EXPECT_EQ((int)matches, 1); 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);
Ready for more advice! https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm (right): https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:22: EXPECT_NE(xmlBody, nil); On 2016/07/22 15:42:38, Mark Mentovai wrote: > I wrote: > > Not sure that you need this independent test case. createReturnsValidXML > > effectively tests this too. If you want to be explicit about it, you can stick > > EXPECT_TRUE(xml_body_) in as the first line of createReturnsValidXML. > > This comment from last time was unaddressed. Oh oops! I recall addressing this somehow. Thanks for the reminder, this is now addressed!
In an effort to make sure I caught all comments, I parsed through all of the comments not responded to in this CR tool. I've got two questions buried below, so I'll iterate them up here for convenience: Fixtures are used when each test requires something -- is the main reason we don't use fixtures in my tests now because the tests have become so concise? (Thanks Mark!) 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? https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/BUILD.gn (right): https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:5: static_library("project") { On 2016/07/15 21:56:08, Mark Mentovai wrote: > We should come up with better names, since these are kind of globally namespaced > within all of Chrome. Prefix things with mac_installer? Acknowledged. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:5: static_library("project") { On 2016/07/15 21:56:08, Mark Mentovai wrote: > We should come up with better names, since these are kind of globally namespaced > within all of Chrome. Prefix things with mac_installer? Done. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:27: executable("downloader_test") { On 2016/07/15 21:56:08, Mark Mentovai wrote: > Probably just mac_installer_test, and we can have all of our tests run from a > single executable. Acknowledged. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:27: executable("downloader_test") { On 2016/07/15 21:56:08, Mark Mentovai wrote: > Probably just mac_installer_test, and we can have all of our tests run from a > single executable. Done. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:30: "//testing/gtest/src/gtest_main.cc", On 2016/07/15 21:56:08, Mark Mentovai wrote: > I think you want to depend on the gtest_main source set, which already compiles > this, rather than compiling another copy in this target. Acknowledged. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:30: "//testing/gtest/src/gtest_main.cc", On 2016/07/15 21:56:08, Mark Mentovai wrote: > I think you want to depend on the gtest_main source set, which already compiles > this, rather than compiling another copy in this target. Done. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm (right): https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:5: #include "../OmahaXMLRequest.h" On 2016/07/15 21:56:08, Mark Mentovai wrote: > #import this one. Also, we should probably move to more absolute paths for > #includes. Acknowledged. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:5: #include "../OmahaXMLRequest.h" On 2016/07/15 21:56:08, Mark Mentovai wrote: > #import this one. Also, we should probably move to more absolute paths for > #includes. Done. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:16: virtual ~OmahaXMLRequestTest() { [xmlBody release]; } On 2016/07/15 21:56:08, Mark Mentovai wrote: > Here’s where scoped_nsobject would be handy: you won’t need to write your own > releases in destructors if you’re using scoped_nsobject<NSXMLDocument> xmlBody_; Acknowledged. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:16: virtual ~OmahaXMLRequestTest() { [xmlBody release]; } On 2016/07/15 21:56:08, Mark Mentovai wrote: > Here’s where scoped_nsobject would be handy: you won’t need to write your own > releases in destructors if you’re using scoped_nsobject<NSXMLDocument> xmlBody_; Done. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:18: NSXMLDocument* xmlBody; On 2016/07/15 21:56:08, Mark Mentovai wrote: > Remember to use trailing underscores for your member variables. Also, use > C++-style naming, which is underscores separating words: xml_body_. > https://google.github.io/styleguide/cppguide.html#Variable_Names Acknowledged. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:18: NSXMLDocument* xmlBody; On 2016/07/15 21:56:09, Mark Mentovai wrote: > This should be in a private: section. > https://google.github.io/styleguide/cppguide.html#Access_Control Acknowledged. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:18: NSXMLDocument* xmlBody; On 2016/07/18 18:15:53, Anna Zeng wrote: > On 2016/07/15 21:56:09, Mark Mentovai wrote: > > This should be in a private: section. > > https://google.github.io/styleguide/cppguide.html#Access_Control > > From the above link: " For technical reasons, we allow data members of a test > fixture class to be protected when using Google Test)." Done. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:19: }; On 2016/07/15 21:56:08, Mark Mentovai wrote: > The private: section should also end with DISALLOW_COPY_AND_ASSIGN. > https://google.github.io/styleguide/cppguide.html#Declaration_Order Acknowledged. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:25: TEST_F(OmahaXMLRequestTest, createReturnsValidXML) { On 2016/07/15 21:56:08, Mark Mentovai wrote: > Test case names should begin with capital letters. This applies in this file and > in the system info test. Done. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/SystemInfo_test.mm (right): https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:5: #include "../SystemInfo.h" On 2016/07/15 21:56:09, Mark Mentovai wrote: > #import Done. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:10: class SystemInfoTest : public ::testing::Test {}; On 2016/07/15 21:56:09, Mark Mentovai wrote: > Don’t need this at all. You haven’t used it, since you’ve only got TEST, not > TEST_F. Done. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:13: EXPECT_NE([SystemInfo getArch], nil); On 2016/07/15 21:56:09, Mark Mentovai wrote: > EXPECT_TRUE is fine. Done. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:21: EXPECT_GE((int)[[SystemInfo getArch] length], 3); On 2016/07/22 13:55:49, Mark Mentovai wrote: > On 2016/07/22 01:45:37, Anna Zeng wrote: > > On 2016/07/15 21:56:09, Mark Mentovai wrote: > > > You’ve got three tests where I think that one will do: > > > > > > - getArchReturnsAtLeastThreeCharacters implies that the arch was a string, > so > > > getArchReturnsAString isn’t necessary > > > - getArchReturnsAString implies that the return was non-nil, so > > > getArchReturnsAString isn’t necessary > > > > > > Actually, I think we should nail the test down a bit harder. We know we only > > > want to see x86_64h or i486 here (ugh). So let’s just test that this returns > > one > > > of those things. We don’t need class tests or length tests or non-nil > checks. > > > EXPECT_TRUE([arch isEqualToString:@"i486"] || [arch > > > isEqualToString:@"x86_64h"]);. If you’re compelled to, you can stick this in > > an > > > #if defined(__x86_64__) and #error on other architectures, the same as in > the > > > .mm file. > > > > > > So you’ll be writing less code but actually testing more. Efficiency! > > > > So... you mean that we want the code to work ONLY on i486 or x86_64, and to > > throw an error for all other architectures? Are... you sure? > > Remember the macro that we stuck at the top of SystemInfo.h? If we ever did find > ourselves building for a non-__x86_64__ system, the .m file would #error too, > and if we fixed that, we’d also want to fix this test. > > We know that i486 and x86_64h are the only things that the API we’re using can > return now. We also know that it’s quirky, because i486 is kind of a weird thing > to return on a 64-bit system to begin with. So we’d just be duplicating that in > our test, basically saying “we know that these are the only two things we’ve > ever seen that weird call return, and if it ever starts returning something > else, we probably want to verify that it’s correct.” Done. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:43: TEST(SystemInfoTest, getOSVersionReturnsNumbers) { On 2016/07/15 21:56:09, Mark Mentovai wrote: > Similar here. Let’s just make sure that the format is "10.digits.digits". > NSRegularExpression might be the easiest way to do this, actually. Again, > probably only this needs one TEST to cover getOSVersion. > > If you want to get fancy, make sure that the runs of digits don’t begin with > zero unless there’s only a single digit in there (as in 10.11.0). Done. https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/requestCheck.dtd (right): https://codereview.chromium.org/2137743002/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/requestCheck.dtd:1: <!ELEMENT request (os, app)> On 2016/07/19 18:45:58, Elly Jones wrote: > can you please comment this file heavily? I (and I think most of the team) can't > read XML DTDs. Done. https://codereview.chromium.org/2137743002/diff/40001/chrome/installer/mac/ap... File chrome/installer/mac/app/BUILD.gn (right): https://codereview.chromium.org/2137743002/diff/40001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:27: executable("mac_installer_test") { On 2016/07/19 21:03:49, Mark Mentovai wrote: > OK! Got a better answer here. > > This should use the test() target type instead of executable(). That should take > care of the gtest dependencies for you. You’ll find examples in .gn files > elsewhere in the codebase. Done. https://codereview.chromium.org/2137743002/diff/40001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:36: "//testing/gtest:gtest_main", On 2016/07/19 21:03:49, Mark Mentovai wrote: > And the answer on this one is to use //base/test:run_all_unittests instead of > gtest_main. (You can also remove the commented-out line 30.) Done. https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm (right): https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:21: EXPECT_TRUE(xml_body_); On 2016/07/22 15:42:38, Mark Mentovai wrote: > (this is what I was talking about—this was unaddressed from the last round of > feedback.) Done. https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:25: NSString* requestDTDLocation = On 2016/07/22 15:42:38, Mark Mentovai wrote: > When you move EXPECT_TRUE(xml_body_) here, you’ll notice that you have this one > test class that does one thing ([OmahaXMLRequest createXMLRequestBody]) for one > caller only (OmahaXMLRequestTest.CreateReturnsValidXML). Seems kinda spread out > and not all that necessary! > > At that point, you don’t need the test class at all! You can just write this as > a TEST() (not TEST_F()) that starts out as: > > base::scoped_nsobject<NSXMLDocument> xml_body( > [OmahaXMLRequest createXMLRequestBody]); > ASSERT_TRUE(xml_body); > > NSString* requestDTDLocation = … > > Note the use of ASSERT_TRUE instead of EXPECT_TRUE now. That’s because if > xml_body is nil, it’s pointless to continue the test. ASSERT_TRUE will stop it > dead in its tracks right there. If we allowed it to continue, surely we’d hit > another error at the EXPECT_TRUE at the bottom of the function, because sending > a message to the nil xml_body would return nil, which would fail the > EXPECT_TRUE. But that second failure would just mask the first, real one, which > is that xml_body was nil to begin with. Thanks Mark! This is a good point here -- if the assertion fails I'll be able to correctly figure out that xml_body was nil. https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/SystemInfo_test.mm (right): https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:11: class SystemInfoTest : public ::testing::Test { On 2016/07/22 15:42:38, Mark Mentovai wrote: > We shouldn’t have a test class here or any TEST_F. Instead… 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? https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:22: EXPECT_TRUE([arch isEqualToString:@"i486"] || On 2016/07/22 15:42:38, Mark Mentovai wrote: > 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? Done. https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:27: NSRegularExpression* regex = [NSRegularExpression On 2016/07/22 15:42:38, Mark Mentovai wrote: > Similar here, just get the OS version right here in this TEST() (not TEST_F()). Done. https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:28: regularExpressionWithPattern:@"^10\\.[0-9]+\\.[0-9]+$" On 2016/07/22 15:42:38, Mark Mentovai wrote: > 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. Oh wow, good point! Done. https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:35: EXPECT_EQ((int)matches, 1); On 2016/07/22 15:42:38, Mark Mentovai wrote: > 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); 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?
https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/requestCheck.dtd (right): https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/requestCheck.dtd:12: Below, this indicates that |protocol| is a required attribute of the Nice documentation :D
I wrote a book for you! https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/SystemInfo_test.mm (right): https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:11: class SystemInfoTest : public ::testing::Test { Anna Zeng wrote: > 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? 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: > A test case contains one or many tests. You should group your tests into test > cases that reflect the structure of the tested code. When multiple tests in a > test case need to share common objects and subroutines, you can put them into > a test fixture class. and then explains a bit more in the “Test Fixtures: Using the Same Data Configuration for Multiple Tests” section. https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:35: EXPECT_EQ((int)matches, 1); Anna Zeng wrote: > 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? 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. https://codereview.chromium.org/2137743002/diff/100001/chrome/installer/mac/a... File chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm (right): https://codereview.chromium.org/2137743002/diff/100001/chrome/installer/mac/a... chrome/installer/mac/app/testing/OmahaXMLRequest_test.mm:9: #import "chrome/installer/mac/app/OmahaXMLRequest.h" This should be the _first_ #import in the file. When you’re testing something, #import/#include the thing that it’s testing first, above everything else (even system includes). The rationale for our #include rules is in the style guide at https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes. Note that the example there says “In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or test the stuff in dir2/foo2.h”. It applies to the test code too! https://codereview.chromium.org/2137743002/diff/100001/chrome/installer/mac/a... File chrome/installer/mac/app/testing/SystemInfo_test.mm (right): https://codereview.chromium.org/2137743002/diff/100001/chrome/installer/mac/a... chrome/installer/mac/app/testing/SystemInfo_test.mm:7: #import "chrome/installer/mac/app/SystemInfo.h" Likewise.
LGTM with those last couple of changes, by the way.
Addendum: When I wrote > 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. I wasn’t being 100% honest. Elly or I would have asked you to turn it into EXPECT_EQ(). Because when EXPECT_TRUE fails, it just tells you that it failed, and you’re stuck scratching your head, probably asking “what’s the value of matches, then?” But when EXPECT_EQ fails, it’ll tell you what the two values were, so you can begin debugging right away.
The CQ bit was checked by zengster@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org Link to the patchset: https://codereview.chromium.org/2137743002/#ps120001 (title: "Last change before LGTM!")
Thanks Mark for your feedback! Submitted to CQ. https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... File chrome/installer/mac/app/testing/SystemInfo_test.mm (right): https://codereview.chromium.org/2137743002/diff/80001/chrome/installer/mac/ap... chrome/installer/mac/app/testing/SystemInfo_test.mm:35: EXPECT_EQ((int)matches, 1); On 2016/07/22 18:03:24, Mark Mentovai wrote: > Anna Zeng wrote: > > 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? > > 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. This is an amazing book Mark! I highly recommend this for publishing on your online column. :D
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zengster@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2137743002/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/BUILD.gn (right): https://codereview.chromium.org/2137743002/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/BUILD.gn:35: deps = [ The trybot output sounds like it’s asking you to depend on //testing/gtest:gtest here.
The CQ bit was checked by zengster@google.com
The CQ bit was unchecked by zengster@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zengster@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zengster@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org Link to the patchset: https://codereview.chromium.org/2137743002/#ps140001 (title: "Fix for the trybot")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Progress!
On 2016/07/25 15:28:12, Mark Mentovai wrote: > Progress! Thanks for the encouragement! :D
The CQ bit was checked by zengster@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org Link to the patchset: https://codereview.chromium.org/2137743002/#ps160001 (title: "Attempt two for the trybots")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zengster@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org Link to the patchset: https://codereview.chromium.org/2137743002/#ps180001 (title: "Added some copyright content to please the trybot lords")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Added test file for SystemInfo.m, on top of the initial commit for Chrome meta installer. Just look at BUILD.gn and SystemInfo_test.mm, everything else is consistent with CL 2094583004 (Initial commit for Chrome meta installer https://codereview.chromium.org/2094583004/) BUG= patch from issue 2106523002 at patchset 60001 (http://crrev.com/2106523002#ps60001) ========== to ========== Added test file for SystemInfo.m, on top of the initial commit for Chrome meta installer. Just look at BUILD.gn and SystemInfo_test.mm, everything else is consistent with CL 2094583004 (Initial commit for Chrome meta installer https://codereview.chromium.org/2094583004/) BUG= patch from issue 2106523002 at patchset 60001 (http://crrev.com/2106523002#ps60001) ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Added test file for SystemInfo.m, on top of the initial commit for Chrome meta installer. Just look at BUILD.gn and SystemInfo_test.mm, everything else is consistent with CL 2094583004 (Initial commit for Chrome meta installer https://codereview.chromium.org/2094583004/) BUG= patch from issue 2106523002 at patchset 60001 (http://crrev.com/2106523002#ps60001) ========== to ========== Added test file for SystemInfo.m, on top of the initial commit for Chrome meta installer. Just look at BUILD.gn and SystemInfo_test.mm, everything else is consistent with CL 2094583004 (Initial commit for Chrome meta installer https://codereview.chromium.org/2094583004/) BUG= patch from issue 2106523002 at patchset 60001 (http://crrev.com/2106523002#ps60001) Committed: https://crrev.com/10fd066ee3eae1695902044db958a85394ad3fe8 Cr-Commit-Position: refs/heads/master@{#407573} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/10fd066ee3eae1695902044db958a85394ad3fe8 Cr-Commit-Position: refs/heads/master@{#407573} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
