PRESUBMIT: Enforce tracker prefix for all BUG entries

PRESUBMIT: Enforce tracker prefix for all BUG entries

Changed function definition from private to public. This was needed to test the function and to maintain the consistency.

BUG=webrtc:8197
NOTRY=True
R=kjellander@webrtc.org

Review-Url: https://codereview.webrtc.org/3010153002 .
Cr-Commit-Position: refs/heads/master@{#19831}
Committed: https://webrtc.googlesource.com/src/+/9893e253f95b2583ae5af3126c68ab52830b7a00

[charujain, 2017-09-05 11:08:22]

Description was changed from

==========
PRESUBMIT: Enforce tracker prefix for all BUG entries

BUG=
==========

to

==========
PRESUBMIT: Enforce tracker prefix for all BUG entries

BUG=webrtc:8197
==========

[charujain, 2017-09-05 11:11:14]

charujain@webrtc.org changed reviewers:
+ charujain@webrtc.org, ehmaldonado@google.com, kjellander@webrtc.org

[charujain, 2017-09-05 11:11:14]

I don't see test file for presubmit. Should we add one for this particular case?

[ehmaldonado_webrtc, 2017-09-05 11:42:44]

On 2017/09/05 11:11:14, charujain wrote:
> I don't see test file for presubmit. Should we add one for this particular
case?

[ehmaldonado_webrtc, 2017-09-05 11:43:03]

ehmaldonado@webrtc.org changed reviewers:
+ ehmaldonado@webrtc.org

[ehmaldonado_webrtc, 2017-09-05 11:43:04]

https://codereview.webrtc.org/3010153002/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.webrtc.org/3010153002/diff/1/PRESUBMIT.py#newcode434
PRESUBMIT.py:434: if 'none'.startswith(bug.lower()):
Why not bug.lower() == 'none'?

[kjellander_webrtc, 2017-09-05 14:37:53]

Good template for test: https://cs.chromium.org/chromium/src/PRESUBMIT_test.py

https://codereview.webrtc.org/3010153002/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.webrtc.org/3010153002/diff/1/PRESUBMIT.py#newcode434
PRESUBMIT.py:434: if 'none'.startswith(bug.lower()):
On 2017/09/05 11:43:04, ehmaldonado_webrtc wrote:
> Why not bug.lower() == 'none'?

Good point. It's copy-pasted from
https://chromium-review.googlesource.com/c/v8/v8/+/454796

[charujain, 2017-09-06 17:54:52]

On 2017/09/05 14:37:53, kjellander_webrtc wrote:
> Good template for test: https://cs.chromium.org/chromium/src/PRESUBMIT_test.py
> 
> https://codereview.webrtc.org/3010153002/diff/1/PRESUBMIT.py
> File PRESUBMIT.py (right):
> 
> https://codereview.webrtc.org/3010153002/diff/1/PRESUBMIT.py#newcode434
> PRESUBMIT.py:434: if 'none'.startswith(bug.lower()):
> On 2017/09/05 11:43:04, ehmaldonado_webrtc wrote:
> > Why not bug.lower() == 'none'?
> 
> Good point. It's copy-pasted from
> https://chromium-review.googlesource.com/c/v8/v8/+/454796

Added test case.

[kjellander_webrtc, 2017-09-06 18:42:09]

Nice with proper tests. Can you please ensure it's also running? 
See comment

https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT.py#newcode496
PRESUBMIT.py:496: test_directories = [
You need to add the root directory here in order to get the unit tests to
actually execute.
You can verify it using git cl presubmit -v (I think)

https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test.py
File PRESUBMIT_test.py (right):

https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test.py#newcode27
PRESUBMIT_test.py:27: 
nit: +1 blank line

https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test_mocks.py
File PRESUBMIT_test_mocks.py (right):

https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test_mocks.py#n...
PRESUBMIT_test_mocks.py:41: 
+blank line

[ehmaldonado_webrtc, 2017-09-06 18:50:55]

https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test.py
File PRESUBMIT_test.py (right):

https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test.py#newcode8
PRESUBMIT_test.py:8: def testCommitMessageBugEntryWithNoError(self):
Check that BUG=None works

https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test_mocks.py
File PRESUBMIT_test_mocks.py (right):

https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test_mocks.py#n...
PRESUBMIT_test_mocks.py:42: class MockChange(object):
Where is this used?

[charujain1, 2017-09-07 08:03:47]

On 2017/09/06 18:50:55, ehmaldonado_webrtc wrote:
> https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test.py
> File PRESUBMIT_test.py (right):
> 
> https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test.py#newcode8
> PRESUBMIT_test.py:8: def testCommitMessageBugEntryWithNoError(self):
> Check that BUG=None works
> 
> https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test_mocks.py
> File PRESUBMIT_test_mocks.py (right):
> 
>
https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test_mocks.py#n...
> PRESUBMIT_test_mocks.py:42: class MockChange(object):
> Where is this used?

Its been used to initialize 
self.change = MockChange([])

[charujain1, 2017-09-07 08:16:18]

On 2017/09/07 08:03:47, charujain1 wrote:
> On 2017/09/06 18:50:55, ehmaldonado_webrtc wrote:
> > https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test.py
> > File PRESUBMIT_test.py (right):
> > 
> >
https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test.py#newcode8
> > PRESUBMIT_test.py:8: def testCommitMessageBugEntryWithNoError(self):
> > Check that BUG=None works
> > 
> > https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test_mocks.py
> > File PRESUBMIT_test_mocks.py (right):
> > 
> >
>
https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test_mocks.py#n...
> > PRESUBMIT_test_mocks.py:42: class MockChange(object):
> > Where is this used?
> 
> Its been used to initialize 
> self.change = MockChange([])

On running 'git cl presubmit -v' i see bunch of errors/warnings from previous
code (https://paste.googleplex.com/5434884885577728).
Do we know why haven't we fixed these?

[kjellander_webrtc, 2017-09-07 08:24:57]

On 2017/09/07 08:16:18, charujain1 wrote:
> On 2017/09/07 08:03:47, charujain1 wrote:
> > On 2017/09/06 18:50:55, ehmaldonado_webrtc wrote:
> > > https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test.py
> > > File PRESUBMIT_test.py (right):
> > > 
> > >
> https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test.py#newcode8
> > > PRESUBMIT_test.py:8: def testCommitMessageBugEntryWithNoError(self):
> > > Check that BUG=None works
> > > 
> > >
https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test_mocks.py
> > > File PRESUBMIT_test_mocks.py (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test_mocks.py#n...
> > > PRESUBMIT_test_mocks.py:42: class MockChange(object):
> > > Where is this used?
> > 
> > Its been used to initialize 
> > self.change = MockChange([])
> 
> On running 'git cl presubmit -v' i see bunch of errors/warnings from previous
> code (https://paste.googleplex.com/5434884885577728).
> Do we know why haven't we fixed these?

Whoa, that was a lot. I guess they might have been ignored or just somehow
didn't show up when I was working on making the PyLint rules stricter a while
back.
Can you fix the few ones related to your files in this CL only?

[kjellander_webrtc, 2017-09-07 08:25:12]

Description was changed from

==========
PRESUBMIT: Enforce tracker prefix for all BUG entries

BUG=webrtc:8197
==========

to

==========
PRESUBMIT: Enforce tracker prefix for all BUG entries

BUG=webrtc:8197
NOTRY=True
==========

[kjellander_webrtc, 2017-09-07 08:28:15]

I added NOTRY=True for you as well, since only the presubmit trybot makes sense
for this CL.

https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test.py
File PRESUBMIT_test.py (right):

https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test.py#newcode8
PRESUBMIT_test.py:8: def testCommitMessageBugEntryWithNoError(self):
On 2017/09/06 18:50:55, ehmaldonado_webrtc wrote:
> Check that BUG=None works

Thanks for remembering this. It would be annoying if we started failing on that
since it still happens every now and then.

https://codereview.webrtc.org/3010153002/diff/100001/PRESUBMIT_test.py
File PRESUBMIT_test.py (right):

https://codereview.webrtc.org/3010153002/diff/100001/PRESUBMIT_test.py#newcode34
PRESUBMIT_test.py:34: self.assertEqual(1, len(errors))
This should not be an error. we want to accept None as a value. So the code
needs to be updated.

[charujain, 2017-09-08 09:56:47]

On 2017/09/07 08:24:57, kjellander_webrtc wrote:
> On 2017/09/07 08:16:18, charujain1 wrote:
> > On 2017/09/07 08:03:47, charujain1 wrote:
> > > On 2017/09/06 18:50:55, ehmaldonado_webrtc wrote:
> > > > https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test.py
> > > > File PRESUBMIT_test.py (right):
> > > > 
> > > >
> >
https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test.py#newcode8
> > > > PRESUBMIT_test.py:8: def testCommitMessageBugEntryWithNoError(self):
> > > > Check that BUG=None works
> > > > 
> > > >
> https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test_mocks.py
> > > > File PRESUBMIT_test_mocks.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.webrtc.org/3010153002/diff/60001/PRESUBMIT_test_mocks.py#n...
> > > > PRESUBMIT_test_mocks.py:42: class MockChange(object):
> > > > Where is this used?
> > > 
> > > Its been used to initialize 
> > > self.change = MockChange([])
> > 
> > On running 'git cl presubmit -v' i see bunch of errors/warnings from
previous
> > code (https://paste.googleplex.com/5434884885577728).
> > Do we know why haven't we fixed these?
> 
> Whoa, that was a lot. I guess they might have been ignored or just somehow
> didn't show up when I was working on making the PyLint rules stricter a while
> back.
> Can you fix the few ones related to your files in this CL only?

The ones related to my change are these:

Ignoring /usr/local/google/home/charujain/webrtc-checkout/src/PRESUBMIT.py; not
a valid file name (cc, h, cpp, cu, cuh)
Done processing
/usr/local/google/home/charujain/webrtc-checkout/src/PRESUBMIT.py
Ignoring /usr/local/google/home/charujain/webrtc-checkout/src/PRESUBMIT_test.py;
not a valid file name (cc, h, cpp, cu, cuh)
Done processing
/usr/local/google/home/charujain/webrtc-checkout/src/PRESUBMIT_test.py
Ignoring
/usr/local/google/home/charujain/webrtc-checkout/src/PRESUBMIT_test_mocks.py;
not a valid file name (cc, h, cpp, cu, cuh)
Done processing
/usr/local/google/home/charujain/webrtc-checkout/src/PRESUBMIT_test_mocks.py

Which is due to Upper case used in file names. I am not sure if we want to
change that since I see convention adopted for PRESUBMIT file names is Upper
case in all the folders.

[charujain, 2017-09-08 09:57:05]

https://codereview.webrtc.org/3010153002/diff/100001/PRESUBMIT_test.py
File PRESUBMIT_test.py (right):

https://codereview.webrtc.org/3010153002/diff/100001/PRESUBMIT_test.py#newcode34
PRESUBMIT_test.py:34: self.assertEqual(1, len(errors))
On 2017/09/07 08:28:15, kjellander_webrtc wrote:
> This should not be an error. we want to accept None as a value. So the code
> needs to be updated.

I think the code already handles this. The test was wrong since i was not
checking for String 'None'. 

Fixed it now.

[kjellander_webrtc, 2017-09-08 13:07:32]

The code was handling it well, yes. Just rename the test method now as it's not
testing what it sounds like

https://codereview.webrtc.org/3010153002/diff/120001/PRESUBMIT_test.py
File PRESUBMIT_test.py (right):

https://codereview.webrtc.org/3010153002/diff/120001/PRESUBMIT_test.py#newcode28
PRESUBMIT_test.py:28: def
testCommitMessageButEntryReturnErrorWhenBugIsNone(self):
testCommitMessageBugEntryIsNone

[charujain, 2017-09-11 13:07:19]

On 2017/09/08 13:07:32, kjellander_webrtc wrote:
> The code was handling it well, yes. Just rename the test method now as it's
not
> testing what it sounds like
> 
> https://codereview.webrtc.org/3010153002/diff/120001/PRESUBMIT_test.py
> File PRESUBMIT_test.py (right):
> 
>
https://codereview.webrtc.org/3010153002/diff/120001/PRESUBMIT_test.py#newcode28
> PRESUBMIT_test.py:28: def
> testCommitMessageButEntryReturnErrorWhenBugIsNone(self):
> testCommitMessageBugEntryIsNone

Done

[kjellander_webrtc, 2017-09-11 19:00:40]

lgtm

[charujain, 2017-09-12 13:28:44]

The CQ bit was checked by charujain@webrtc.org

[commit-bot: I haz the power, 2017-09-12 13:28:53]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-12 13:33:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-09-12 13:33:43]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/21499)

[charujain1, 2017-09-13 08:24:19]

charujain@google.com changed reviewers:
+ phoglund@webrtc.org

[charujain1, 2017-09-13 08:24:20]

Patrik, The presubmit check fails with following error: "W: 12,13: Access to a
protected member _CheckCommitMessageBugEntry of a client class
(protected-access)".

Since the function we are testing here is private (_). But shouldn't it be OK to
suppress that in tests or is it better to make it public?

[charujain, 2017-09-14 08:16:35]

Description was changed from

==========
PRESUBMIT: Enforce tracker prefix for all BUG entries

BUG=webrtc:8197
NOTRY=True
==========

to

==========
PRESUBMIT: Enforce tracker prefix for all BUG entries

Changed function definition from private to public. This was needed to test the
function and to maintain the consistency.

BUG=webrtc:8197
NOTRY=True
==========

[charujain, 2017-09-14 08:39:31]

On 2017/09/13 08:24:20, charujain1 wrote:
> Patrik, The presubmit check fails with following error: "W: 12,13: Access to a
> protected member _CheckCommitMessageBugEntry of a client class
> (protected-access)".
> 
> Since the function we are testing here is private (_). But shouldn't it be OK
to
> suppress that in tests or is it better to make it public?

Fixed the warning by making the function call public.

[charujain, 2017-09-14 09:06:38]

The CQ bit was checked by charujain@webrtc.org

[charujain, 2017-09-14 09:06:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3010153002/#ps200001 (title:
"Made private function public.")

[commit-bot: I haz the power, 2017-09-14 09:06:52]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-14 09:10:03]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1505379998210190,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"00ef23f42c5b6e28a6853b798e638cce29af66b1"}

[commit-bot: I haz the power, 2017-09-14 09:10:04]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1505379998210190,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"00ef23f42c5b6e28a6853b798e638cce29af66b1"}

[commit-bot: I haz the power, 2017-09-14 09:10:06]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1505379998210190,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"00ef23f42c5b6e28a6853b798e638cce29af66b1"}

[commit-bot: I haz the power, 2017-09-14 09:10:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-09-14 09:10:09]

Failed to commit the patch.

[charujain, 2017-09-14 09:14:08]

The CQ bit was checked by charujain@webrtc.org

[charujain, 2017-09-14 09:14:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3010153002/#ps220001 (title:
"gclient sync")

[commit-bot: I haz the power, 2017-09-14 09:14:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-14 09:17:53]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1505380448715780,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"68462f99e969ad8c894c47f5ceba5c468acb77f1"}

[commit-bot: I haz the power, 2017-09-14 09:18:00]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1505380448715780,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"68462f99e969ad8c894c47f5ceba5c468acb77f1"}

[commit-bot: I haz the power, 2017-09-14 09:18:04]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1505380448715780,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"68462f99e969ad8c894c47f5ceba5c468acb77f1"}

[commit-bot: I haz the power, 2017-09-14 09:18:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-09-14 09:18:07]

Failed to commit the patch.

[charujain, 2017-09-14 11:33:26]

Description was changed from

==========
PRESUBMIT: Enforce tracker prefix for all BUG entries

Changed function definition from private to public. This was needed to test the
function and to maintain the consistency.

BUG=webrtc:8197
NOTRY=True
==========

to

==========
PRESUBMIT: Enforce tracker prefix for all BUG entries

Changed function definition from private to public. This was needed to test the
function and to maintain the consistency.

BUG=webrtc:8197
NOTRY=True
R=kjellander@webrtc.org

Review-Url: https://codereview.webrtc.org/3010153002 .
Cr-Commit-Position: refs/heads/master@{#19831}
Committed:
https://webrtc.googlesource.com/src/+/9893e253f95b2583ae5af3126c68ab52830b7a00
==========

[charujain, 2017-09-14 11:33:28]

Committed patchset #12 (id:220001) manually as
9893e253f95b2583ae5af3126c68ab52830b7a00.