New issue
Advanced search Search tips

Issue 890657 link

Starred by 2 users
Status: Verified
Owner: ----
Closed: Oct 1
Cc:
tkent@chromium.org
kojii@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

PRESUBMIT_test.py is failing

Project Member Reported by fergal@chromium.org, Oct 1 
It looks like it was broken by eaca74262314335fe003c1198b03f4d2d82e9a2e on Mar 12 but it may be that that was fixed and now it's just broken again. At that CL there are actually 2 failures. At head, there is just 1.


$@ python ./third_party/WebKit/PRESUBMIT_test.py 
F.
======================================================================
FAIL: testCheckChangeOnUploadWithEmptyAffectedFileList (__main__.PresubmitTest)
This verifies that CheckChangeOnUpload will skip calling
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "./third_party/WebKit/PRESUBMIT_test.py", line 82, in testCheckChangeOnUploadWithEmptyAffectedFileList
    subprocess.Popen.assert_not_called()
  File "/usr/lib/python2.7/dist-packages/mock/mock.py", line 897, in assert_not_called
    raise AssertionError(msg)
AssertionError: Expected 'Popen' to not have been called. Called 1 times.

Comment 1 by fergal@chromium.org, Oct 1 

Oh one correction. 

both

python ./third_party/blink/PRESUBMIT_test.py 

and

python ./third_party/WebKit/PRESUBMIT_test.py 

are failing with the same error but the WebKit was originally failing with 2 errors at eaca74262314335fe003c1198b03f4d2d82e9a2e.

Comment 2 by tkent@chromium.org, Oct 1 

> python ./third_party/blink/PRESUBMIT_test.py 
> and
> python ./third_party/WebKit/PRESUBMIT_test.py 

I saw no errors for both of them with today's ToT on Linux and macOS.

Comment 3 by fergal@chromium.org, Oct 1

Cc: kojii@chromium.org
Root cause is that

https://cs.chromium.org/chromium/src/third_party/pymock/mock.py

does not have an assert_not_called method! So

        # pylint: disable=E1101                                                                      
        subprocess.Popen.assert_not_called()

is not actually asserting anything, assert_not_called is just treated as any other mocked method. The pylint was correct.

The reason it failed for me is because I have mock.py installed on my workstation and it was being used instead of the chromium one. So I was getting a real assert_not_called.

So the test is broken but also the code _is_ broken, Popen is being called with 3 files. In the old version of the code, these 3 files were filtered out by

       # Filter out files that follow Chromium's coding style.                                      
        if re_chromium_style_file.search(file_path):
            continue

but that has been removed.
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 1 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4fc8a2120b2fcb3e998c358a568893d9767ba698

commit 4fc8a2120b2fcb3e998c358a568893d9767ba698
Author: Fergal Daly <fergal@chromium.org>
Date: Mon Oct 01 08:43:26 2018

Fix blink/PRESUBMIT_test

This test was using mock.py's assert_not_called method but that does
not exist in the version in third_party/pymock. The result is that the
assert call was mocked out, allowing the test to pass without actually
testing anything.

Fix is two-fold:
1 factor out the filtering code and test it directly
2 assert that .call_count is 0

Bug:  890657 
Change-Id: Iffc6382ad0d9335a11c17f5ffe4d0c6ae6cbcf6c
Reviewed-on: https://chromium-review.googlesource.com/1253601
Commit-Queue: Fergal Daly <fergal@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595406}
[modify] https://crrev.com/4fc8a2120b2fcb3e998c358a568893d9767ba698/third_party/blink/PRESUBMIT.py
[modify] https://crrev.com/4fc8a2120b2fcb3e998c358a568893d9767ba698/third_party/blink/PRESUBMIT_test.py

Comment 5 by fergal@chromium.org, Oct 1

Status: Verified (was: Available)
►

Sign in to add a comment