Warn about calls to *ForTesting and about undefined histograms in production Java code |
|||||
Issue descriptionFor (Objective-)C++, there is _CheckNoProductionCodeUsingTestOnlyFunctions in //PRESUBMIT.py which can detect when a method with a ForTesting suffix is referenced in production code. Checking this would also be useful for Java files, and a quick and dirty check showed that the above presubmit would indeed handle .java. This bug tracks adding such similar presubmit check for .java files. Notes: * This will likely end up in chrome/android/java/src/PRESUBMIT.py. * The check will be a modified copy of the one for C++, because some details are simpler for Java (e.g., no need to consider *_for_testing as well, no need to figure out what is production code, because mostly anything inside java/ is). * The check will be a presubmit error on upload but not on commit, because there are false positives to be expected. Similarly, there is _CheckUmaHistogramChanges for C++ code, which would be beneficial to have for Java as well. This bug tracks that too.
,
Mar 16 2018
Three more preparation CLs in flight right now: https://crrev.com/c/966502, https://crrev.com/c/966626, https://crrev.com/c/966906. After those, I'll actually do the one to complete this task. :) And after that, perhaps also adding something to check histograms (see the discussion in https://crrev.com/c/964842 & https://crbug.com/821823#c1 ).
,
Mar 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d5de76a8f40f419243777dbeb73c126d0829ab01 commit d5de76a8f40f419243777dbeb73c126d0829ab01 Author: Vaclav Brozek <vabr@chromium.org> Date: Sat Mar 17 07:57:50 2018 Fix pylint issues in PRESUBMIT.py pylint reports a number of issues for PRESUBMIT.py. One of them is a real error (using an undefined variable instead of a file name), the rest just nits. Still, good to fix them all to allow pylint discover new failures easily in the future. Bug: 821981 Change-Id: I857be0b4072bfe41687c52d38335d5efebcc952f Reviewed-on: https://chromium-review.googlesource.com/966502 Commit-Queue: Vaclav Brozek <vabr@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#543934} [modify] https://crrev.com/d5de76a8f40f419243777dbeb73c126d0829ab01/PRESUBMIT.py
,
Mar 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cdc7defb018587e3dd6ce6da9e962c281f34b725 commit cdc7defb018587e3dd6ce6da9e962c281f34b725 Author: Vaclav Brozek <vabr@chromium.org> Date: Tue Mar 20 09:54:35 2018 Run PRESUBMIT_test.py in subdirectories Right now, changes to //PRESUBMIT.py trigger running //PRESUBMIT_test.py during presubmit checks. However, this does not happen for PRESUBMITs in subdirectories, of which there is a number with associated PRESUBMIT_test. This CL allows running the associated PRESUBMIT_test. Bug: 821981 Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I057f4d736992795dde7dd0d329ed3773d22553dc Reviewed-on: https://chromium-review.googlesource.com/966906 Commit-Queue: Vaclav Brozek <vabr@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Cr-Commit-Position: refs/heads/master@{#544318} [modify] https://crrev.com/cdc7defb018587e3dd6ce6da9e962c281f34b725/PRESUBMIT.py [modify] https://crrev.com/cdc7defb018587e3dd6ce6da9e962c281f34b725/chrome/browser/resources/PRESUBMIT_test.py [modify] https://crrev.com/cdc7defb018587e3dd6ce6da9e962c281f34b725/ios/PRESUBMIT_test.py [modify] https://crrev.com/cdc7defb018587e3dd6ce6da9e962c281f34b725/media/PRESUBMIT_test.py [modify] https://crrev.com/cdc7defb018587e3dd6ce6da9e962c281f34b725/third_party/WebKit/PRESUBMIT_test.py
,
Mar 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/680f13310569fb0a57b18b5515c98353051b906a commit 680f13310569fb0a57b18b5515c98353051b906a Author: Vaclav Brozek <vabr@chromium.org> Date: Tue Mar 20 14:11:19 2018 Fix and test c/android/java/src PRESUBMIT chrome/android/java/src/PRESUBMIT.py currently contains one test: checking for bad use of Notification.Builder. This CL improves that test in two ways: * It makes it ignore the forbidden pattern in comments. * It adds a test for the presubmit check. Bug: 821981 Change-Id: I5e2e362ed2168e479b446eccdca11e76a95e9e85 Reviewed-on: https://chromium-review.googlesource.com/966626 Commit-Queue: Vaclav Brozek <vabr@chromium.org> Reviewed-by: Bernhard Bauer <bauerb@chromium.org> Cr-Commit-Position: refs/heads/master@{#544348} [modify] https://crrev.com/680f13310569fb0a57b18b5515c98353051b906a/chrome/android/java/src/PRESUBMIT.py [add] https://crrev.com/680f13310569fb0a57b18b5515c98353051b906a/chrome/android/java/src/PRESUBMIT_test.py
,
Mar 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8a8e2e2058afe6f1b6a2e39163536197725d0d63 commit 8a8e2e2058afe6f1b6a2e39163536197725d0d63 Author: Vaclav Brozek <vabr@chromium.org> Date: Fri Mar 23 22:01:06 2018 Improve _CheckUmaHistogramChanges presubmit The _CheckUmaHistogramChanges checks whether newly added UMA_HISTOGRAM_* calls only use defined histogram names. This CL removes two ways to introduce false positives: * Unrelated macro names which contain UMA_HISTOGRAM as a continuous substring * More than one string literal on the line with the histogram name. While those cases are rare, there seems no downside to fixing them. Bug: 821981 Change-Id: Ie1a803f562883f567d577e742ed2ed87fd0dfe66 Reviewed-on: https://chromium-review.googlesource.com/978245 Commit-Queue: Vaclav Brozek <vabr@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#545583} [modify] https://crrev.com/8a8e2e2058afe6f1b6a2e39163536197725d0d63/PRESUBMIT.py [modify] https://crrev.com/8a8e2e2058afe6f1b6a2e39163536197725d0d63/PRESUBMIT_test.py
,
Mar 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0e730cbd89e2f054b910d11481debcc826792e93 commit 0e730cbd89e2f054b910d11481debcc826792e93 Author: Vaclav Brozek <vabr@chromium.org> Date: Sat Mar 24 06:18:17 2018 Improve presubmit for histogram name typos Currently, _CheckUmaHistogramChanges detects the cases like calling UMA_HISTOGRAM_BOOLEAN("NonexistingHistogram.Typo", true) when "NonexistingHistogram.Typo" is not a histogram name defined in histograms.xml. However, it won't detect the case when the name is after a line break: UMA_HISTOGRAM_BOOLEAN( "NonexistingHistogram.VeeeeeeeryLoooooooongName.WithSubitems", true) This will be often the case once the check gets extended to Java, where the UMA_HISTOGRAM* macros are replaced with the RecordHistogram.record*Histogram methods, which have longer names. Bug: 821981 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I71d01f3b7012e8a8d6c4628d67a470c57005cd56 Reviewed-on: https://chromium-review.googlesource.com/978219 Commit-Queue: Vaclav Brozek <vabr@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#545676} [modify] https://crrev.com/0e730cbd89e2f054b910d11481debcc826792e93/PRESUBMIT.py [modify] https://crrev.com/0e730cbd89e2f054b910d11481debcc826792e93/PRESUBMIT_test.py
,
Mar 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bdac817c6876a327ff1e938abf990256b0db8743 commit bdac817c6876a327ff1e938abf990256b0db8743 Author: Vaclav Brozek <vabr@chromium.org> Date: Sat Mar 24 06:30:47 2018 Extend histogram presubmit check to Java. Currently, _CheckUmaHistogramChanges detects use of undefined histogram names in (Objective-)C++ files. This CL extends that support to Java as well. Bug: 821981 Change-Id: Ida931db191f0793927b0e957e64bf6dac699d502 Reviewed-on: https://chromium-review.googlesource.com/978163 Commit-Queue: Vaclav Brozek <vabr@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#545678} [modify] https://crrev.com/bdac817c6876a327ff1e938abf990256b0db8743/PRESUBMIT.py [modify] https://crrev.com/bdac817c6876a327ff1e938abf990256b0db8743/PRESUBMIT_test.py
,
Mar 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7dbc28c91fa05d715d7b1da282aef7ceeb4c5449 commit 7dbc28c91fa05d715d7b1da282aef7ceeb4c5449 Author: Vaclav Brozek <vabr@chromium.org> Date: Tue Mar 27 08:35:23 2018 Add presubmit for Java against *ForTesting in production For (Objective-)C++, there is _CheckNoProductionCodeUsingTestOnlyFunctions in //PRESUBMIT.py which can detect when a method with a ForTesting suffix is referenced in production code. This bug tracks adding such similar presubmit check for production .java files, which are approximated as .java files with no "test" or "junit" in the path and filename. The check is a modified copy of the one for C++. It does not attempt to share code with the C++ check, because some details are simpler for Java (e.g., no need to consider *_for_testing as well). The check is just a presubmit prompt, not error, because there are false positives to be expected. Bug: 821981 Change-Id: I3bb137197070ac696fbe0fd35d50abbfb823a8d8 Reviewed-on: https://chromium-review.googlesource.com/977581 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: agrieve <agrieve@chromium.org> Reviewed-by: Bernhard Bauer <bauerb@chromium.org> Commit-Queue: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#546063} [modify] https://crrev.com/7dbc28c91fa05d715d7b1da282aef7ceeb4c5449/PRESUBMIT.py [modify] https://crrev.com/7dbc28c91fa05d715d7b1da282aef7ceeb4c5449/PRESUBMIT_test.py
,
Mar 27 2018
,
Mar 27 2018
It took a little more CLs than anticipated, but the (extended) task is now done.
,
Mar 27 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by bugdroid1@chromium.org
, Mar 16 2018