|
|
Patch Set 1 #Patch Set 2 : Just a simple modification to the Info.plist #Patch Set 3 : Converted FrameworkSample to Google Style guidelines, also fixed a buffer overflow. #Patch Set 4 : Simplified the test code to remove intermediate values" #
Total messages: 3
|
Preston
I'm adding an example for using Google Test with a Framework in Xcode. Please let ...
|
17 years, 5 months ago (2008年08月16日 03:28:19 UTC) #1 | |||||||||||||||||||||||||||||||||||||||
I'm adding an example for using Google Test with a Framework in Xcode. Please let me know what you think.
http://codereview.appspot.com/2947/diff/22/14 File xcode/Samples/FrameworkSample/Info.plist (right): http://codereview.appspot.com/2947/diff/22/14#newcode20 Line 20: <string>1.01</string> Do we need to manually update this with each point release? That doesn't scale. http://codereview.appspot.com/2947/diff/22/13 File xcode/Samples/FrameworkSample/Widget.cpp (right): http://codereview.appspot.com/2947/diff/22/13#newcode33 Line 33: // Widget.cpp gtest uses the .cc extension for C++ code. Can we be consistent here? http://codereview.appspot.com/2947/diff/22/15 File xcode/Samples/FrameworkSample/Widget.h (right): http://codereview.appspot.com/2947/diff/22/15#newcode45 Line 45: Widget(int number, std::string name); Change the second argument to pass-by-reference? http://codereview.appspot.com/2947/diff/22/15#newcode49 Line 49: float floatValue(); Style: GetFloatValue(). The same for other members. http://codereview.appspot.com/2947/diff/22/15#newcode58 Line 58: float _number; Style: number_ http://codereview.appspot.com/2947/diff/22/15#newcode59 Line 59: std::string _name; Style: name_ http://codereview.appspot.com/2947/diff/22/12 File xcode/Samples/FrameworkSample/Widget_test.cpp (right): http://codereview.appspot.com/2947/diff/22/12#newcode33 Line 33: // Widget_test.cpp .cc? http://codereview.appspot.com/2947/diff/22/12#newcode40 Line 40: // This test verifies that the constructor sets the internal state of the Style: add a blank line before this. http://codereview.appspot.com/2947/diff/22/12#newcode47 Line 47: EXPECT_EQ(widget.floatValue(), value); The first argument to EXPECT_EQ() is the expected value. Therefore you should switch the two arguments. The behavior of comparing two floating-point values directly is not well defined. Therefore use EXPECT_FLOAT_EQ instead. I'd get rid of the intermediate variables as they actually make the test harder to follow. Just: Widget widget(1.0f, "name"); EXPECT_FLOAT_EQ(1.0f, widget.GetFloatValue()); EXPECT_EQ("name", widget.GetStringValue()); The same for the next test. http://codereview.appspot.com/2947/diff/22/12#newcode63 Line 63: int main(int argc, char **argv) { We want to promote gtest_main. Can we instead make the test link with gtest_main? http://codereview.appspot.com/2947/diff/22/11 File xcode/gtest.xcodeproj/project.pbxproj (right): http://codereview.appspot.com/2947/diff/22/11#newcode101 Line 101: remoteGlobalIDString = 40C44ADC0E3798F4008FCC51; Is this change intentional?
Zhanyong, I converted the files to Google Style (and fixed a buffer overflow) can you take a look at the new version. Thanks, Preston http://codereview.appspot.com/2947/diff/22/14 File xcode/Samples/FrameworkSample/Info.plist (right): http://codereview.appspot.com/2947/diff/22/14#newcode20 Line 20: <string>1.01</string> Nope. This is just a dummy value of the version of the WidgetFramework, not Google Test. On 2008年08月18日 21:32:31, Zhanyong wrote: > Do we need to manually update this with each point release? That doesn't scale. http://codereview.appspot.com/2947/diff/22/13 File xcode/Samples/FrameworkSample/Widget.cpp (right): http://codereview.appspot.com/2947/diff/22/13#newcode33 Line 33: // Widget.cpp On 2008年08月18日 21:32:31, Zhanyong wrote: > gtest uses the .cc extension for C++ code. Can we be consistent here? Done. http://codereview.appspot.com/2947/diff/22/15 File xcode/Samples/FrameworkSample/Widget.h (right): http://codereview.appspot.com/2947/diff/22/15#newcode45 Line 45: Widget(int number, std::string name); On 2008年08月18日 21:32:31, Zhanyong wrote: > Change the second argument to pass-by-reference? Done. http://codereview.appspot.com/2947/diff/22/15#newcode49 Line 49: float floatValue(); On 2008年08月18日 21:32:31, Zhanyong wrote: > Style: GetFloatValue(). > > The same for other members. Done. http://codereview.appspot.com/2947/diff/22/15#newcode58 Line 58: float _number; On 2008年08月18日 21:32:31, Zhanyong wrote: > Style: number_ Done. http://codereview.appspot.com/2947/diff/22/15#newcode59 Line 59: std::string _name; On 2008年08月18日 21:32:31, Zhanyong wrote: > Style: name_ Done. http://codereview.appspot.com/2947/diff/22/12 File xcode/Samples/FrameworkSample/Widget_test.cpp (right): http://codereview.appspot.com/2947/diff/22/12#newcode33 Line 33: // Widget_test.cpp On 2008年08月18日 21:32:31, Zhanyong wrote: > .cc? Done. http://codereview.appspot.com/2947/diff/22/12#newcode40 Line 40: // This test verifies that the constructor sets the internal state of the On 2008年08月18日 21:32:31, Zhanyong wrote: > Style: add a blank line before this. Done. http://codereview.appspot.com/2947/diff/22/12#newcode47 Line 47: EXPECT_EQ(widget.floatValue(), value); On 2008年08月18日 21:32:31, Zhanyong wrote: > The first argument to EXPECT_EQ() is the expected value. Therefore you should > switch the two arguments. > > The behavior of comparing two floating-point values directly is not well > defined. Therefore use EXPECT_FLOAT_EQ instead. > > I'd get rid of the intermediate variables as they actually make the test harder > to follow. Just: > > Widget widget(1.0f, "name"); > EXPECT_FLOAT_EQ(1.0f, widget.GetFloatValue()); > EXPECT_EQ("name", widget.GetStringValue()); > > The same for the next test. Done. http://codereview.appspot.com/2947/diff/22/12#newcode63 Line 63: int main(int argc, char **argv) { On 2008年08月18日 21:32:31, Zhanyong wrote: > We want to promote gtest_main. Can we instead make the test link with > gtest_main? Done. http://codereview.appspot.com/2947/diff/22/11 File xcode/gtest.xcodeproj/project.pbxproj (right): http://codereview.appspot.com/2947/diff/22/11#newcode101 Line 101: remoteGlobalIDString = 40C44ADC0E3798F4008FCC51; No, it was a mistake. Reverted.