Remove the Singleton<T> presubmit check from presubmit_canned_checks.py

Remove the Singleton<T> presubmit check from presubmit_canned_checks.py
This check is specific to Chromium codebase, there's no point in having it in depot_tools.

BUG=None
R=maruel@chromium.org

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

[Alexander Potapenko, 2015-02-13 13:04:39]

glider@chromium.org changed reviewers:
+ jochen@chromium.org

[Alexander Potapenko, 2015-02-13 13:04:40]

Jochen, can you please take a look?

The presubmit warning I'm trying to fix prevents me from committing
https://codereview.chromium.org/790473002/

[Alexander Potapenko, 2015-02-13 16:53:42]

glider@chromium.org changed reviewers:
+ maruel@chromium.org
- jochen@chromium.org

[Alexander Potapenko, 2015-02-13 16:55:06]

Jochen is OOO.
Marc-Antoine, may I ask you to review this?

[M-A Ruel, 2015-02-13 17:04:18]

https://codereview.chromium.org/924863002/diff/1/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/924863002/diff/1/presubmit_canned_checks.py#n...
presubmit_canned_checks.py:973: def CheckSingletonInHeaders(input_api,
output_api, source_file_filter=None):
I wonder why this check is in presubmit_canned_checks at all. It's highly
chromium src.git specific.

https://codereview.chromium.org/924863002/diff/1/presubmit_canned_checks.py#n...
presubmit_canned_checks.py:982: 'base', 'memory', 'singleton.h')
You can't hardcode src.git paths here.

[Alexander Potapenko, 2015-02-13 17:10:12]

On 2015/02/13 17:04:18, M-A Ruel wrote:
> https://codereview.chromium.org/924863002/diff/1/presubmit_canned_checks.py
> File presubmit_canned_checks.py (right):
> 
>
https://codereview.chromium.org/924863002/diff/1/presubmit_canned_checks.py#n...
> presubmit_canned_checks.py:973: def CheckSingletonInHeaders(input_api,
> output_api, source_file_filter=None):
> I wonder why this check is in presubmit_canned_checks at all. It's highly
> chromium src.git specific.
> 
>
https://codereview.chromium.org/924863002/diff/1/presubmit_canned_checks.py#n...
> presubmit_canned_checks.py:982: 'base', 'memory', 'singleton.h')
> You can't hardcode src.git paths here.

Agreed. Ok if I move this check to src.git then?

[M-A Ruel, 2015-02-13 17:27:41]

Yes

Sent by touching glass
Le 2015-02-13 12:10, <glider@chromium.org> a écrit :

> On 2015/02/13 17:04:18, M-A Ruel wrote:
>
>> https://codereview.chromium.org/924863002/diff/1/
>> presubmit_canned_checks.py
>> File presubmit_canned_checks.py (right):
>>
>
>
> https://codereview.chromium.org/924863002/diff/1/
> presubmit_canned_checks.py#newcode973
>
>> presubmit_canned_checks.py:973: def CheckSingletonInHeaders(input_api,
>> output_api, source_file_filter=None):
>> I wonder why this check is in presubmit_canned_checks at all. It's highly
>> chromium src.git specific.
>>
>
>
> https://codereview.chromium.org/924863002/diff/1/
> presubmit_canned_checks.py#newcode982
>
>> presubmit_canned_checks.py:982: 'base', 'memory', 'singleton.h')
>> You can't hardcode src.git paths here.
>>
>
> Agreed. Ok if I move this check to src.git then?
>
> https://codereview.chromium.org/924863002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexander Potapenko, 2015-02-18 13:44:19]

I'm adding this check to src/PRESUBMIT.py in
https://codereview.chromium.org/933253002/
But I cannot cleanly remove it from presubmit_canned_checks.py (see patch set 2)
because of a presubmit test failure:

tests/presubmit_unittest.py (0.38s) failed
.............................................................F....................................................
======================================================================
FAIL: testPanProjectChecks (__main__.CannedChecksUnittest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/google/ssd/depot_tools/third_party/pymox/mox.py", line 1623,
in new_method
    mox_obj.VerifyAll()
  File "/usr/local/google/ssd/depot_tools/third_party/pymox/mox.py", line 212,
in VerifyAll
    mock_obj._Verify()
  File "/usr/local/google/ssd/depot_tools/third_party/pymox/mox.py", line 380,
in _Verify
    raise ExpectedMethodCallsError(self._expected_calls_queue)
ExpectedMethodCallsError: Verify: Expected methods never called:
  0.  AffectedSourceFiles(<IgnoreArg>) -> [<MockAnything instance>]

Mocks in testPanProjectChecks() are emitting .py paths, so I'm not sure which of
them I was expected to remove.
Also it's not really clear how the mock calls are mapped to individual test
cases.
Can you share any insight about that?

[M-A Ruel, 2015-02-18 13:49:40]

https://codereview.chromium.org/924863002/diff/20001/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

https://codereview.chromium.org/924863002/diff/20001/tests/presubmit_unittest...
tests/presubmit_unittest.py:2841: for _ in range(3):
change 3 to 2.

pymox is such a bag of hurt.

[Alexander Potapenko, 2015-02-18 13:59:03]

You bet.

diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py
index 1cb0119..88eda79 100755
--- a/tests/presubmit_unittest.py
+++ b/tests/presubmit_unittest.py
@@ -2838,7 +2838,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
         'foo1', 'description1', self.fake_root_dir, None, 0, 0, None)
     input_api = self.MockInputApi(change, False)
     affected_file = self.mox.CreateMock(presubmit.SvnAffectedFile)
-    for _ in range(3):
+    for _ in range(2):
       input_api.AffectedFiles(file_filter=mox.IgnoreArg(),
include_deletes=False
           ).AndReturn([affected_file])
       affected_file.LocalPath()


$ git cl presubmit --force
(CUT)
tests/presubmit_unittest.py (0.37s) failed
.............................................................F....................................................
======================================================================
FAIL: testPanProjectChecks (__main__.CannedChecksUnittest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/google/ssd/depot_tools/third_party/pymox/mox.py", line 1618,
in new_method
    func(self, *args, **kwargs)
  File "tests/presubmit_unittest.py", line 2868, in testPanProjectChecks
    owners_check=False)
  File "/usr/local/google/ssd/depot_tools/presubmit_canned_checks.py", line
1054, in PanProjectChecks
    input_api, output_api, source_file_filter=sources))
  File "/usr/local/google/ssd/depot_tools/presubmit_canned_checks.py", line 318,
in CheckChangeHasNoStrayWhitespace
    input_api, source_file_filter)
  File "/usr/local/google/ssd/depot_tools/presubmit_canned_checks.py", line 263,
in _FindNewViolationsOfRule
    file_filter=source_file_filter):
  File "/usr/local/google/ssd/depot_tools/third_party/pymox/mox.py", line 771,
in __call__
    expected_method = self._VerifyMethodCall()
  File "/usr/local/google/ssd/depot_tools/third_party/pymox/mox.py", line 827,
in _VerifyMethodCall
    raise UnexpectedMethodCallError(self, expected)
UnexpectedMethodCallError: Unexpected method call.  unexpected:-  expected:+
- AffectedFiles(file_filter=<function <lambda> at 0x7fd74b4540c8>,
include_deletes=False) -> None
+ AffectedSourceFiles(<IgnoreArg>) -> [<MockAnything instance>]

----------------------------------------------------------------------



Removing lines 2857-2858 helped, but I'm not sure I haven't poured the baby out.

[M-A Ruel, 2015-02-18 14:06:47]

https://codereview.chromium.org/924863002/diff/20001/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

https://codereview.chromium.org/924863002/diff/20001/tests/presubmit_unittest...
tests/presubmit_unittest.py:2854:
input_api.AffectedSourceFiles(mox.IgnoreArg()).AndReturn([affected_file])
It was this one in fact, since it's an extraneous AffectedSourceFiles, not a
AffectedFiles.

[Alexander Potapenko, 2015-02-18 16:05:49]

Nope:

FAIL: testPanProjectChecks (__main__.CannedChecksUnittest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/google/ssd/depot_tools/third_party/pymox/mox.py", line 1618,
in new_method
    func(self, *args, **kwargs)
  File "tests/presubmit_unittest.py", line 2868, in testPanProjectChecks
    owners_check=False)
  File "/usr/local/google/ssd/depot_tools/presubmit_canned_checks.py", line
1057, in PanProjectChecks
    input_api, output_api, source_file_filter=sources))
  File "/usr/local/google/ssd/depot_tools/presubmit_canned_checks.py", line 957,
in _CheckConstNSObject
    for f in input_api.AffectedSourceFiles(objective_c_filter):
  File "/usr/local/google/ssd/depot_tools/third_party/pymox/mox.py", line 771,
in __call__
    expected_method = self._VerifyMethodCall()
  File "/usr/local/google/ssd/depot_tools/third_party/pymox/mox.py", line 827,
in _VerifyMethodCall
    raise UnexpectedMethodCallError(self, expected)
UnexpectedMethodCallError: Unexpected method call.  unexpected:-  expected:+
- AffectedSourceFiles(<function objective_c_filter at 0x7f1ba396b398>) -> None
+ ReadFile(<MockAnything instance>) -> 'Hey!\nHo!\nHey!\nHo!\n\n'

[M-A Ruel, 2015-02-18 16:08:15]

https://codereview.chromium.org/924863002/diff/20001/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

https://codereview.chromium.org/924863002/diff/20001/tests/presubmit_unittest...
tests/presubmit_unittest.py:2855:
input_api.ReadFile(affected_file).AndReturn('Hey!\nHo!\nHey!\nHo!\n\n')
Then you need to remove this one too. Basically you need to remove all the
equivalent calls that used to be in lines 2090-2124.

[Alexander Potapenko, 2015-02-19 14:43:10]

PS3 seems to be the minimal patch, the test seems to pass now.

[M-A Ruel, 2015-02-19 14:43:40]

lgtm

[M-A Ruel, 2015-02-19 14:43:57]

On 2015/02/19 14:43:40, M-A Ruel wrote:
> lgtm

Oh, not lgtm

[M-A Ruel, 2015-02-19 14:44:27]

On 2015/02/19 14:43:57, M-A Ruel wrote:
> On 2015/02/19 14:43:40, M-A Ruel wrote:
> > lgtm
> 
> Oh, not lgtm

because you need to keep CheckSingletonInHeaders() to be compatible with the old
code for a while, just leave the function there.

[Alexander Potapenko, 2015-02-19 14:47:49]

On 2015/02/19 14:44:27, M-A Ruel wrote:
> On 2015/02/19 14:43:57, M-A Ruel wrote:
> > On 2015/02/19 14:43:40, M-A Ruel wrote:
> > > lgtm
> > 
> > Oh, not lgtm
> 
> because you need to keep CheckSingletonInHeaders() to be compatible with the
old
> code for a while, just leave the function there.

Do you suspect someone is calling CheckSingletonInHeaders() from their presubmit
scripts?

[M-A Ruel, 2015-02-19 14:50:29]

On 2015/02/19 14:47:49, Alexander Potapenko wrote:
> On 2015/02/19 14:44:27, M-A Ruel wrote:
> > On 2015/02/19 14:43:57, M-A Ruel wrote:
> > > On 2015/02/19 14:43:40, M-A Ruel wrote:
> > > > lgtm
> > > 
> > > Oh, not lgtm
> > 
> > because you need to keep CheckSingletonInHeaders() to be compatible with the
> old
> > code for a while, just leave the function there.
> 
> Do you suspect someone is calling CheckSingletonInHeaders() from their
presubmit
> scripts?

Yes. Better be safe than revert.

[Alexander Potapenko, 2015-02-19 15:02:13]

Haha, pymox tried to check that function when I restored it:

FAIL: testMembersChanged (__main__.CannedChecksUnittest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/google/ssd/depot_tools/third_party/pymox/mox.py", line 1618,
in new_method
    func(self, *args, **kwargs)
  File "tests/presubmit_unittest.py", line 1892, in testMembersChanged
    self.compareMembers(presubmit_canned_checks, members)
  File "/usr/local/google/ssd/depot_tools/testing_support/super_mox.py", line
78, in compareMembers
    self.assertEqual(actual_members, expected_members)
AssertionError: Lists differ: ['CheckBuildbotPendingBuilds',... !=
['CheckBuildbotPendingBuilds',...

How about I make it a dummy then (PS4)?

[M-A Ruel, 2015-02-19 16:12:03]

https://codereview.chromium.org/924863002/diff/60001/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/924863002/diff/60001/presubmit_canned_checks....
presubmit_canned_checks.py:975: return []
return [
  output_api.PresubmitNotifyResult(
    'CheckSingletonInHeaders is deprecated, please remove it.')
]

this way the check is not silently removed and this reduces surprise to
downstream users.

[Alexander Potapenko, 2015-02-19 16:27:21]

https://codereview.chromium.org/924863002/diff/60001/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/924863002/diff/60001/presubmit_canned_checks....
presubmit_canned_checks.py:975: return []
On 2015/02/19 16:12:03, M-A Ruel wrote:
> return [
>   output_api.PresubmitNotifyResult(
>     'CheckSingletonInHeaders is deprecated, please remove it.')
> ]
> 
> this way the check is not silently removed and this reduces surprise to
> downstream users.

Done.

[M-A Ruel, 2015-02-19 16:27:59]

lgtm

[Alexander Potapenko, 2015-02-19 16:29:55]

The CQ bit was checked by glider@chromium.org

[commit-bot: I haz the power, 2015-02-19 16:30:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/924863002/80001

[commit-bot: I haz the power, 2015-02-19 16:33:36]

Committed patchset #5 (id:80001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=294124