Issue 4628980216889344: Issue #1234 - Add unit tests for TrimString (Closed)
DescriptionIssue #1234 - Add unit tests for TrimString
Add unit tests for TrimString(). This function already existed in the code,
but also replaces CString member function Trim().
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #MessagesTotal messages: 10
This change is really in anticipation of adding the other replacements for CString member functions.
http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... File test/UtilTest.cpp (right): http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... test/UtilTest.cpp:22: //---------------------------------- I would say we don't need these comments http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... test/UtilTest.cpp:23: namespace { { should be on the new line http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... test/UtilTest.cpp:31: TEST( Trim, trim_00 ) here, above and below: we don't use spaces after ( and before )
http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... File test/UtilTest.cpp (right): http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... test/UtilTest.cpp:22: //---------------------------------- On 2015/01/05 15:27:43, sergei wrote: > I would say we don't need these comments We'll need these once the other unit tests land in this file. http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... test/UtilTest.cpp:23: namespace { On 2015/01/05 15:27:43, sergei wrote: > { should be on the new line Done. http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... test/UtilTest.cpp:31: TEST( Trim, trim_00 ) On 2015/01/05 15:27:43, sergei wrote: > here, above and below: we don't use spaces after ( and before ) Done.
http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... File test/UtilTest.cpp (right): http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... test/UtilTest.cpp:22: //---------------------------------- On 2015/01/05 16:25:07, Eric wrote: > On 2015/01/05 15:27:43, sergei wrote: > > I would say we don't need these comments > > > We'll need these once the other unit tests land in this file. Why will we need it? We don't use them in other files with tests. It may be even better to put other tests into another file. http://codereview.adblockplus.org/4628980216889344/diff/5724160613416960/test... File test/UtilTest.cpp (right): http://codereview.adblockplus.org/4628980216889344/diff/5724160613416960/test... test/UtilTest.cpp:32: TEST(Trim, trim_00) We also usually add "Test" to the test case, thus call it like `TEST(TrimTest, trim_00)`.
http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... File test/UtilTest.cpp (right): http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... test/UtilTest.cpp:22: //---------------------------------- On 2015/01/05 16:51:03, sergei wrote: > Why will we need it? We don't use them in other files with tests. I've got around another 70 tests already written (from the ATL conversion work a few months ago) for various utility functions we'll need to finish off CString. It's much easier to find the tests you're looking for with the separators. > It may be even better to put other tests into another file. The file structure for test we have mirrors that of the source structure. If we had a build system that made it easier to deal with multiple test harnesses, I'd be inclined to arrange the tests differently. You've got to compile the whole executable with all the test sources anyway. As it is, though, I don't see any value splitting out the tests into separate files at this point. http://codereview.adblockplus.org/4628980216889344/diff/5724160613416960/test... File test/UtilTest.cpp (right): http://codereview.adblockplus.org/4628980216889344/diff/5724160613416960/test... test/UtilTest.cpp:32: TEST(Trim, trim_00) On 2015/01/05 16:51:03, sergei wrote: > We also usually add "Test" to the test case, thus call it like `TEST(TrimTest, > trim_00)`. We have, and it's entirely redundant to do so. It's in a file named ...Test.cpp. It's in a declaration TEST. When you see the names in test output, it's completely clear that they're tests. Personally, I'd rather ditch the suffix for test case identifiers in the other tests, although that's low priority. If you can identify a downside of getting rid of the suffix, please let me know.
http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... File test/UtilTest.cpp (right): http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... test/UtilTest.cpp:22: //---------------------------------- On 2015/01/05 17:18:25, Eric wrote: > On 2015/01/05 16:51:03, sergei wrote: > > Why will we need it? We don't use them in other files with tests. > > I've got around another 70 tests already written (from the ATL conversion work a > few months ago) for various utility functions we'll need to finish off CString. > It's much easier to find the tests you're looking for with the separators. > > > It may be even better to put other tests into another file. > > The file structure for test we have mirrors that of the source structure. If we > had a build system that made it easier to deal with multiple test harnesses, I'd > be inclined to arrange the tests differently. You've got to compile the whole > executable with all the test sources anyway. As it is, though, I don't see any > value splitting out the tests into separate files at this point. > It still does not convince me, let's wait for @Oleksandr's or @Felix' response. http://codereview.adblockplus.org/4628980216889344/diff/5724160613416960/test... File test/UtilTest.cpp (right): http://codereview.adblockplus.org/4628980216889344/diff/5724160613416960/test... test/UtilTest.cpp:32: TEST(Trim, trim_00) On 2015/01/05 17:18:25, Eric wrote: > On 2015/01/05 16:51:03, sergei wrote: > > We also usually add "Test" to the test case, thus call it like `TEST(TrimTest, > > trim_00)`. > > We have, and it's entirely redundant to do so. It's in a file named ...Test.cpp. > It's in a declaration TEST. When you see the names in test output, it's > completely clear that they're tests. > > Personally, I'd rather ditch the suffix for test case identifiers in the other > tests, although that's low priority. > > If you can identify a downside of getting rid of the suffix, please let me know. Consistency
http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... File test/UtilTest.cpp (right): http://codereview.adblockplus.org/4628980216889344/diff/5629499534213120/test... test/UtilTest.cpp:22: //---------------------------------- On 2015/01/05 21:13:53, sergei wrote: > On 2015/01/05 17:18:25, Eric wrote: > > On 2015/01/05 16:51:03, sergei wrote: > > > Why will we need it? We don't use them in other files with tests. > > > > I've got around another 70 tests already written (from the ATL conversion work > a > > few months ago) for various utility functions we'll need to finish off > CString. > > It's much easier to find the tests you're looking for with the separators. > > > > > It may be even better to put other tests into another file. > > > > The file structure for test we have mirrors that of the source structure. If > we > > had a build system that made it easier to deal with multiple test harnesses, > I'd > > be inclined to arrange the tests differently. You've got to compile the whole > > executable with all the test sources anyway. As it is, though, I don't see any > > value splitting out the tests into separate files at this point. > > > > It still does not convince me, let's wait for @Oleksandr's or @Felix' response. If we add 70 more tests in this file it will be hardly readable. Comments like this won't help much in that case, IMHO. I'd vote for having more files instead. http://codereview.adblockplus.org/4628980216889344/diff/5724160613416960/test... File test/UtilTest.cpp (right): http://codereview.adblockplus.org/4628980216889344/diff/5724160613416960/test... test/UtilTest.cpp:32: TEST(Trim, trim_00) I agree with Sergei. Also, TEST(TrimTest, Trim00) would be more in line with our previous tests and our style guide.
New patch set
LGTM
LGTM 
|
