Add check to help prevent test-only function usage in production code.

Add check to help prevent test-only function usage in production code.

BUG=none
TEST=Manual testing was performed. Warning message from presubmit test includes note to report issues to joi@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96184

[Jói, 2011-08-05 11:09:10]

As promised in
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

Cheers,
Jói

[M-A Ruel, 2011-08-05 18:27:41]

False positives have a high cost, is there any at the moment?

http://codereview.chromium.org/7582002/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/7582002/diff/1/PRESUBMIT.py#newcode65
PRESUBMIT.py:65: not file_exclusion_pattern.search(local_path)):
It should use a source filter instead and you need to use _EXCLUDED_PATHS.

http://codereview.chromium.org/7582002/diff/1/PRESUBMIT.py#newcode66
PRESUBMIT.py:66: lines = input_api.ReadFile(f).split('\n')
splitlines()

http://codereview.chromium.org/7582002/diff/1/PRESUBMIT.py#newcode77
PRESUBMIT.py:77: 'You might be calling functions intended only for testing
from\n' +
no need for +

[Jói, 2011-08-08 23:11:00]

To test whether there are many false positives, I made a changelist that
included all .cc, .h and .mm files under src/base, src/v8 and
src/third_party/WebKit/Source/WebCore.  There were no false positives when
running a presubmit check over all of these files.

There are two possible false positives that I know about:
a) A top-level function (not a class member) that has a parameter list long
enough to be multi-line; and
b) A call within one "for testing" function to another "for testing" function.

For the former, I could put in some more complicated multi-line parsing logic,
but would rather avoid it just to keep the presubmit check simple.  For the
latter, a best effort could be made with regexp-based parsing, but for the real
thing you would need a proper C++ parser like Clang.

Given that our current code base seems to comply I would suggest keeping things
as simple as possible until we have a proven need for something more expensive.

Cheers,
Jói

http://codereview.chromium.org/7582002/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/7582002/diff/1/PRESUBMIT.py#newcode65
PRESUBMIT.py:65: not file_exclusion_pattern.search(local_path)):
On 2011/08/05 18:27:41, Marc-Antoine Ruel wrote:
> It should use a source filter instead and you need to use _EXCLUDED_PATHS.

Done.

http://codereview.chromium.org/7582002/diff/1/PRESUBMIT.py#newcode66
PRESUBMIT.py:66: lines = input_api.ReadFile(f).split('\n')
On 2011/08/05 18:27:41, Marc-Antoine Ruel wrote:
> splitlines()

Done.

http://codereview.chromium.org/7582002/diff/1/PRESUBMIT.py#newcode77
PRESUBMIT.py:77: 'You might be calling functions intended only for testing
from\n' +
On 2011/08/05 18:27:41, Marc-Antoine Ruel wrote:
> no need for +

Done.

[Jói, 2011-08-09 22:32:56]

maruel: ping

On Mon, Aug 8, 2011 at 11:11 PM,  <joi@chromium.org> wrote:
> To test whether there are many false positives, I made a changelist that
> included all .cc, .h and .mm files under src/base, src/v8 and
> src/third_party/WebKit/Source/WebCore.  There were no false positives when
> running a presubmit check over all of these files.
>
> There are two possible false positives that I know about:
> a) A top-level function (not a class member) that has a parameter list long
> enough to be multi-line; and
> b) A call within one "for testing" function to another "for testing"
> function.
>
> For the former, I could put in some more complicated multi-line parsing
> logic,
> but would rather avoid it just to keep the presubmit check simple.  For the
> latter, a best effort could be made with regexp-based parsing, but for the
> real
> thing you would need a proper C++ parser like Clang.
>
> Given that our current code base seems to comply I would suggest keeping
> things
> as simple as possible until we have a proven need for something more
> expensive.
>
> Cheers,
> Jói
>
>
>
> http://codereview.chromium.org/7582002/diff/1/PRESUBMIT.py
> File PRESUBMIT.py (right):
>
> http://codereview.chromium.org/7582002/diff/1/PRESUBMIT.py#newcode65
> PRESUBMIT.py:65: not file_exclusion_pattern.search(local_path)):
> On 2011/08/05 18:27:41, Marc-Antoine Ruel wrote:
>>
>> It should use a source filter instead and you need to use
>
> _EXCLUDED_PATHS.
>
> Done.
>
> http://codereview.chromium.org/7582002/diff/1/PRESUBMIT.py#newcode66
> PRESUBMIT.py:66: lines = input_api.ReadFile(f).split('\n')
> On 2011/08/05 18:27:41, Marc-Antoine Ruel wrote:
>>
>> splitlines()
>
> Done.
>
> http://codereview.chromium.org/7582002/diff/1/PRESUBMIT.py#newcode77
> PRESUBMIT.py:77: 'You might be calling functions intended only for
> testing from\n' +
> On 2011/08/05 18:27:41, Marc-Antoine Ruel wrote:
>>
>> no need for +
>
> Done.
>
> http://codereview.chromium.org/7582002/
>

[M-A Ruel, 2011-08-10 15:08:06]

lgtm

sorry for the delay, I wasn't convinced until I reread your comment and realized
I had misread it; e.g. there is no false positive; I had somehow read it the
reverse.

[Jói, 2011-08-10 15:11:46]

:-)  Thanks, no problem.

On Wed, Aug 10, 2011 at 3:08 PM,  <maruel@chromium.org> wrote:
> lgtm
>
> sorry for the delay, I wasn't convinced until I reread your comment and
> realized
> I had misread it; e.g. there is no false positive; I had somehow read it the
> reverse.
>
> http://codereview.chromium.org/7582002/
>