Fix CheckDanglingUntriaged in PRESUBMIT.py
Due to not having type check in python, both the implementation and the test were assuming `OldContents()` and `NewContents()` to be string. They are list of strings instead. The function `count(str)` being defined on both, it was passing locally, when I was using DanglingUntriaged on its own line. Bug: chromium:1395297 Change-Id: I2b20e92e2dfd0054bb4b2c3630f35af20661bbdc Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5012813 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/main@{#1222807}
This commit is contained in:
Arthur Sonzogni
committed by
Chromium LUCI CQ
Chromium LUCI CQ
parent
9c26bb7ec4
commit
9eafd22a23
35
PRESUBMIT.py
35
PRESUBMIT.py
@ -7160,7 +7160,7 @@ def CheckDanglingUntriaged(input_api, output_api):
|
||||
# `win-presubmit` are particularly sensitive to reading the files. Adding
|
||||
# this check caused the bot to run 2x longer. See https://crbug.com/1486612.
|
||||
if input_api.no_diffs:
|
||||
return []
|
||||
return []
|
||||
|
||||
def FilterFile(file):
|
||||
return input_api.FilterSourceFile(
|
||||
@ -7171,8 +7171,8 @@ def CheckDanglingUntriaged(input_api, output_api):
|
||||
|
||||
count = 0
|
||||
for f in input_api.AffectedSourceFiles(FilterFile):
|
||||
count -= f.OldContents().count("DanglingUntriaged")
|
||||
count += f.NewContents().count("DanglingUntriaged")
|
||||
count -= sum([l.count("DanglingUntriaged") for l in f.OldContents()])
|
||||
count += sum([l.count("DanglingUntriaged") for l in f.NewContents()])
|
||||
|
||||
# Most likely, nothing changed:
|
||||
if count == 0:
|
||||
@ -7180,10 +7180,7 @@ def CheckDanglingUntriaged(input_api, output_api):
|
||||
|
||||
# Congrats developers for improving it:
|
||||
if count < 0:
|
||||
message = (
|
||||
f"DanglingUntriaged pointers removed: {-count}",
|
||||
f"Thank you!",
|
||||
)
|
||||
message = f"DanglingUntriaged pointers removed: {-count}\nThank you!"
|
||||
return [output_api.PresubmitNotifyResult(message)]
|
||||
|
||||
# Check for 'DanglingUntriaged-notes' in the description:
|
||||
@ -7199,18 +7196,18 @@ def CheckDanglingUntriaged(input_api, output_api):
|
||||
return []
|
||||
|
||||
message = (
|
||||
"Unexpected new occurrences of `DanglingUntriaged` detected. Please",
|
||||
"avoid adding new ones",
|
||||
"",
|
||||
"See documentation:",
|
||||
"https://chromium.googlesource.com/chromium/src/+/main/docs/dangling_ptr.md",
|
||||
"",
|
||||
"See also the guide to fix dangling pointers:",
|
||||
"https://chromium.googlesource.com/chromium/src/+/main/docs/dangling_ptr_guide.md",
|
||||
"",
|
||||
"To disable this warning, please add in the commit description:",
|
||||
"DanglingUntriaged-notes: <rational for new untriaged dangling "
|
||||
"pointers>",
|
||||
"Unexpected new occurrences of `DanglingUntriaged` detected. Please\n" +
|
||||
"avoid adding new ones\n" +
|
||||
"\n" +
|
||||
"See documentation:\n" +
|
||||
"https://chromium.googlesource.com/chromium/src/+/main/docs/dangling_ptr.md\n" +
|
||||
"\n" +
|
||||
"See also the guide to fix dangling pointers:\n" +
|
||||
"https://chromium.googlesource.com/chromium/src/+/main/docs/dangling_ptr_guide.md\n" +
|
||||
"\n" +
|
||||
"To disable this warning, please add in the commit description:\n" +
|
||||
"DanglingUntriaged-notes: <rational for new untriaged dangling " +
|
||||
"pointers>"
|
||||
)
|
||||
return [output_api.PresubmitPromptWarning(message)]
|
||||
|
||||
|
||||
@ -5106,8 +5106,8 @@ class CheckDanglingUntriagedTest(unittest.TestCase):
|
||||
mock_input_api.files = [
|
||||
MockAffectedFile(
|
||||
local_path="foo/foo.cc",
|
||||
old_contents="raw_ptr<T>",
|
||||
new_contents="raw_ptr<T, DanglingUntriaged>",
|
||||
old_contents=["raw_ptr<T>"],
|
||||
new_contents=["raw_ptr<T, DanglingUntriaged>"],
|
||||
)
|
||||
]
|
||||
msgs = PRESUBMIT.CheckDanglingUntriaged(mock_input_api, mock_output_api)
|
||||
@ -5128,18 +5128,15 @@ class CheckDanglingUntriagedTest(unittest.TestCase):
|
||||
mock_input_api.files = [
|
||||
MockAffectedFile(
|
||||
local_path="foo/foo.cc",
|
||||
old_contents="raw_ptr<T>",
|
||||
new_contents="raw_ptr<T, DanglingUntriaged>",
|
||||
old_contents=["raw_ptr<T>"],
|
||||
new_contents=["raw_ptr<T, DanglingUntriaged>"],
|
||||
)
|
||||
]
|
||||
msgs = PRESUBMIT.CheckDanglingUntriaged(mock_input_api,
|
||||
mock_output_api)
|
||||
self.assertEqual(len(msgs), 1)
|
||||
self.assertEqual(len(msgs[0].message), 11)
|
||||
self.assertEqual(
|
||||
msgs[0].message[0],
|
||||
"Unexpected new occurrences of `DanglingUntriaged` detected. Please",
|
||||
)
|
||||
self.assertTrue(("Unexpected new occurrences of `DanglingUntriaged` detected"
|
||||
in msgs[0].message))
|
||||
|
||||
def testNonCppFile(self):
|
||||
"""Test patch adding dangling pointers are not reported in non C++ files"""
|
||||
@ -5150,8 +5147,8 @@ class CheckDanglingUntriagedTest(unittest.TestCase):
|
||||
mock_input_api.files = [
|
||||
MockAffectedFile(
|
||||
local_path="foo/README.md",
|
||||
old_contents="",
|
||||
new_contents="The DanglingUntriaged annotation means",
|
||||
old_contents=[""],
|
||||
new_contents=["The DanglingUntriaged annotation means"],
|
||||
)
|
||||
]
|
||||
msgs = PRESUBMIT.CheckDanglingUntriaged(mock_input_api,
|
||||
@ -5167,8 +5164,8 @@ class CheckDanglingUntriagedTest(unittest.TestCase):
|
||||
mock_input_api.files = [
|
||||
MockAffectedFile(
|
||||
local_path="foo/foo.cc",
|
||||
old_contents="raw_ptr<T>",
|
||||
new_contents="raw_ptr<T, DanglingUntriaged>",
|
||||
old_contents=["raw_ptr<T>"],
|
||||
new_contents=["raw_ptr<T, DanglingUntriaged>"],
|
||||
)
|
||||
]
|
||||
mock_input_api.change.DescriptionText = lambda: (
|
||||
@ -5186,8 +5183,8 @@ class CheckDanglingUntriagedTest(unittest.TestCase):
|
||||
mock_input_api.files = [
|
||||
MockAffectedFile(
|
||||
local_path="foo/foo.cc",
|
||||
old_contents="raw_ptr<T>",
|
||||
new_contents="raw_ptr<T, DanglingUntriaged>",
|
||||
old_contents=["raw_ptr<T>"],
|
||||
new_contents=["raw_ptr<T, DanglingUntriaged>"],
|
||||
)
|
||||
]
|
||||
mock_input_api.change.DescriptionText = lambda: "description"
|
||||
@ -5204,8 +5201,8 @@ class CheckDanglingUntriagedTest(unittest.TestCase):
|
||||
mock_input_api.files = [
|
||||
MockAffectedFile(
|
||||
local_path="foo/foo.cc",
|
||||
old_contents="raw_ptr<T, DanglingUntriaged>",
|
||||
new_contents="raw_ptr<T>",
|
||||
old_contents=["raw_ptr<T, DanglingUntriaged>"],
|
||||
new_contents=["raw_ptr<T>"],
|
||||
)
|
||||
]
|
||||
mock_input_api.change.DescriptionText = lambda: (
|
||||
@ -5225,14 +5222,14 @@ class CheckDanglingUntriagedTest(unittest.TestCase):
|
||||
mock_input_api.files = [
|
||||
MockAffectedFile(
|
||||
local_path="foo/foo.cc",
|
||||
old_contents="raw_ptr<T, DanglingUntriaged>",
|
||||
new_contents="",
|
||||
old_contents=["raw_ptr<T, DanglingUntriaged>"],
|
||||
new_contents=[""],
|
||||
action="D",
|
||||
),
|
||||
MockAffectedFile(
|
||||
local_path="foo/foo.cc",
|
||||
old_contents="",
|
||||
new_contents="raw_ptr<T, DanglingUntriaged>",
|
||||
old_contents=[""],
|
||||
new_contents=["raw_ptr<T, DanglingUntriaged>"],
|
||||
action="A",
|
||||
),
|
||||
]
|
||||
|
||||
Reference in New Issue
Block a user