Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2005023002:
Blimp presubmit to check for unit tests on new .cc files. (Closed)
|
Created:
4 years, 7 months ago by Brian Goldman Modified:
4 years, 6 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBlimp presubmit to check for unit tests on new .cc files.
BUG=612498, 543268
TEST=Added the files blimp/science.cc, blimp/science_unittest.cc, blimp/science2.cc, blimp/science2_unittest.cc, blimp/science_browsertest.cc, and science.cc. Verified that blimp/science.cc was flagged for missing unit tests.
Committed: https://crrev.com/18b2942b8f1890e8307cde644b448830a7a89411
Cr-Commit-Position: refs/heads/master@{#397773}
Patch Set 1 #
Total comments: 16
Patch Set 2 : #Messages
Total messages: 20 (9 generated)
Description was changed from ========== Blimp presubmit to check for unit tests on new .cc files. BUG=612498 TEST=Added blimp/engine/science.cc and verified warning, added blimp/science_unittest.cc and verified warning, added blimp/engine/science_unittest.cc and verified no warning ========== to ========== Blimp presubmit to check for unit tests on new .cc files. BUG=612498 TEST=Added blimp/engine/science.cc and verified warning, added blimp/science_unittest.cc and verified warning, added blimp/engine/science_unittest.cc and verified no warning. ==========
Description was changed from ========== Blimp presubmit to check for unit tests on new .cc files. BUG=612498 TEST=Added blimp/engine/science.cc and verified warning, added blimp/science_unittest.cc and verified warning, added blimp/engine/science_unittest.cc and verified no warning. ========== to ========== Blimp presubmit to check for unit tests on new .cc files. BUG=612498 TEST=Added blimp/engine/science.cc and verified warning, added blimp/science_unittest.cc and verified warning, added blimp/engine/science_unittest.cc and verified no warning. Ironically, this script itself could use some unit tests since it is tedious to test manually, but I don't see a way to unit-test presubmit scripts. ==========
bgoldman@chromium.org changed reviewers: + kmarshall@chromium.org, wez@chromium.org
Description was changed from ========== Blimp presubmit to check for unit tests on new .cc files. BUG=612498 TEST=Added blimp/engine/science.cc and verified warning, added blimp/science_unittest.cc and verified warning, added blimp/engine/science_unittest.cc and verified no warning. Ironically, this script itself could use some unit tests since it is tedious to test manually, but I don't see a way to unit-test presubmit scripts. ========== to ========== Blimp presubmit to check for unit tests on new .cc files. Ironically, this script itself could use some unit tests since it is tedious to test manually, but I don't see a way to unit-test presubmit scripts. BUG=612498 TEST=Added blimp/engine/science.cc and verified warning, added blimp/science_unittest.cc and verified warning, added blimp/engine/science_unittest.cc and verified no warning. ==========
https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py File blimp/PRESUBMIT.py (right): https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode23 blimp/PRESUBMIT.py:23: unittest_files = set() Does this check files that live outside of blimp/ ? https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode25 blimp/PRESUBMIT.py:25: Also need something analogous for Java tests, which have a different directory structure and file naming convention. https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode29 blimp/PRESUBMIT.py:29: if re.search(r'_unittest\.cc$', name): Can we be consistent with search vs match? https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode29 blimp/PRESUBMIT.py:29: if re.search(r'_unittest\.cc$', name): Should we also exclude anything with "test" in the name? Otherwise a foo_browser_test.cc will need a foo_browser_test_unittest.cc . Turtles all the way down~
Suggest including 543268 in the BUG= line.
Sweet! https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py File blimp/PRESUBMIT.py (right): https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode23 blimp/PRESUBMIT.py:23: unittest_files = set() On 2016/05/23 23:57:39, Kevin M wrote: > Does this check files that live outside of blimp/ ? path/PRESUBMIT.py only gets called for files that live under path/, I think? https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode24 blimp/PRESUBMIT.py:24: files_needing_unittest = set() nit: How about new_unittest_files and new_source_files for consistency? https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode25 blimp/PRESUBMIT.py:25: On 2016/05/23 23:57:39, Kevin M wrote: > Also need something analogous for Java tests, which have a different directory > structure and file naming convention. Feels like a follow-up CL? https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode29 blimp/PRESUBMIT.py:29: if re.search(r'_unittest\.cc$', name): On 2016/05/23 23:57:39, Kevin M wrote: > Should we also exclude anything with "test" in the name? Otherwise a > foo_browser_test.cc will need a foo_browser_test_unittest.cc . Turtles all the > way down~ This is only generating a warning, though, so it won't stop CQ processing the CL; question is how many *test*.cc files are complex enough test classes that they need dedicated tests themselves. ;) Presumably not very many..? https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode29 blimp/PRESUBMIT.py:29: if re.search(r'_unittest\.cc$', name): On 2016/05/23 23:57:39, Kevin M wrote: > Can we be consistent with search vs match? And use re.compile() to compile the REs once each, rather than once-per-file. https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode34 blimp/PRESUBMIT.py:34: missing = [] nit: missing_unittest_files?
Description was changed from ========== Blimp presubmit to check for unit tests on new .cc files. Ironically, this script itself could use some unit tests since it is tedious to test manually, but I don't see a way to unit-test presubmit scripts. BUG=612498 TEST=Added blimp/engine/science.cc and verified warning, added blimp/science_unittest.cc and verified warning, added blimp/engine/science_unittest.cc and verified no warning. ========== to ========== Blimp presubmit to check for unit tests on new .cc files. Ironically, this script itself could use some unit tests since it is tedious to test manually, but I don't see a way to unit-test presubmit scripts. BUG=612498 TEST=Added the files blimp/science.cc, blimp/science_unittest.cc, blimp/science2.cc, blimp/science2_unittest.cc, blimp/science_browsertest.cc, and science.cc. Verified that blimp/science.cc was flagged for missing unit tests. ==========
https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py File blimp/PRESUBMIT.py (right): https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode23 blimp/PRESUBMIT.py:23: unittest_files = set() On 2016/05/24 00:44:54, Wez wrote: > On 2016/05/23 23:57:39, Kevin M wrote: > > Does this check files that live outside of blimp/ ? > > path/PRESUBMIT.py only gets called for files that live under path/, I think? I tested this. It only checks files under blimp. https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode24 blimp/PRESUBMIT.py:24: files_needing_unittest = set() On 2016/05/24 00:44:54, Wez wrote: > nit: How about new_unittest_files and new_source_files for consistency? I'm going to leave the names as-is even though I don't particularly like them. source_file is a name used elsewhere in this function to mean any new/modified/deleted file in the changelist, so I think using it to mean specifically a .cc file that needs a unittest.cc file would be confusing. https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode25 blimp/PRESUBMIT.py:25: On 2016/05/24 00:44:54, Wez wrote: > On 2016/05/23 23:57:39, Kevin M wrote: > > Also need something analogous for Java tests, which have a different directory > > structure and file naming convention. > > Feels like a follow-up CL? Yes :) https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode29 blimp/PRESUBMIT.py:29: if re.search(r'_unittest\.cc$', name): On 2016/05/23 23:57:39, Kevin M wrote: > Can we be consistent with search vs match? I ended up getting rid of regex for this section entirely and just startswith and endswith. https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode29 blimp/PRESUBMIT.py:29: if re.search(r'_unittest\.cc$', name): On 2016/05/24 00:44:54, Wez wrote: > On 2016/05/23 23:57:39, Kevin M wrote: > > Should we also exclude anything with "test" in the name? Otherwise a > > foo_browser_test.cc will need a foo_browser_test_unittest.cc . Turtles all the > > way down~ > > This is only generating a warning, though, so it won't stop CQ processing the > CL; question is how many *test*.cc files are complex enough test classes that > they need dedicated tests themselves. ;) Presumably not very many..? I'm going with the assumption that if it ends in "test.cc" then it's a test, and skipping those from the check, since we will have browser tests (although I'm not sure if they will always live with the things they are testing?) Other things that support testing might have "test" in the name, but not likely at the end, if I understand the convention for this repo. https://codereview.chromium.org/2005023002/diff/1/blimp/PRESUBMIT.py#newcode29 blimp/PRESUBMIT.py:29: if re.search(r'_unittest\.cc$', name): On 2016/05/24 00:44:54, Wez wrote: > On 2016/05/23 23:57:39, Kevin M wrote: > > Can we be consistent with search vs match? > > And use re.compile() to compile the REs once each, rather than once-per-file. No more regexes in this section. I do use one for substitution a little further down, but I'm going to go ahead and say regex compilation is no big deal here due to very very small N, and inlining the pattern makes it easier to read.
Description was changed from ========== Blimp presubmit to check for unit tests on new .cc files. Ironically, this script itself could use some unit tests since it is tedious to test manually, but I don't see a way to unit-test presubmit scripts. BUG=612498 TEST=Added the files blimp/science.cc, blimp/science_unittest.cc, blimp/science2.cc, blimp/science2_unittest.cc, blimp/science_browsertest.cc, and science.cc. Verified that blimp/science.cc was flagged for missing unit tests. ========== to ========== Blimp presubmit to check for unit tests on new .cc files. BUG=612498,543268 TEST=Added the files blimp/science.cc, blimp/science_unittest.cc, blimp/science2.cc, blimp/science2_unittest.cc, blimp/science_browsertest.cc, and science.cc. Verified that blimp/science.cc was flagged for missing unit tests. ==========
On 2016/05/24 00:31:20, Wez wrote: > Suggest including 543268 in the BUG= line. Done.
lgtm
LGTM
The CQ bit was checked by bgoldman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2005023002/20001
Message was sent while issue was closed.
Description was changed from ========== Blimp presubmit to check for unit tests on new .cc files. BUG=612498,543268 TEST=Added the files blimp/science.cc, blimp/science_unittest.cc, blimp/science2.cc, blimp/science2_unittest.cc, blimp/science_browsertest.cc, and science.cc. Verified that blimp/science.cc was flagged for missing unit tests. ========== to ========== Blimp presubmit to check for unit tests on new .cc files. BUG=612498,543268 TEST=Added the files blimp/science.cc, blimp/science_unittest.cc, blimp/science2.cc, blimp/science2_unittest.cc, blimp/science_browsertest.cc, and science.cc. Verified that blimp/science.cc was flagged for missing unit tests. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Blimp presubmit to check for unit tests on new .cc files. BUG=612498,543268 TEST=Added the files blimp/science.cc, blimp/science_unittest.cc, blimp/science2.cc, blimp/science2_unittest.cc, blimp/science_browsertest.cc, and science.cc. Verified that blimp/science.cc was flagged for missing unit tests. ========== to ========== Blimp presubmit to check for unit tests on new .cc files. BUG=612498,543268 TEST=Added the files blimp/science.cc, blimp/science_unittest.cc, blimp/science2.cc, blimp/science2_unittest.cc, blimp/science_browsertest.cc, and science.cc. Verified that blimp/science.cc was flagged for missing unit tests. Committed: https://crrev.com/18b2942b8f1890e8307cde644b448830a7a89411 Cr-Commit-Position: refs/heads/master@{#397773} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/18b2942b8f1890e8307cde644b448830a7a89411 Cr-Commit-Position: refs/heads/master@{#397773} |
