Don't require translation screenshots on description-only changes
* This is a follow-up to crrev.com/c/2328669 with further fine tuning from findings in https://crbug.com/1103844#c22. * The current translation screenshot presubmit check flags modified messages: 1) if the message does not yet have a screenshot, or 2) if the message content itself changed. * This CL changes the behavior to for modified messages check in this order: 1) if the message content itself changed -> flag the message. 2) if the message meaning changed and there is neither an existing screenshot nor is a new one added with the CL -> flag the message. * Important cases to consider: messages that don't yet have a screenshot and for which only the message meaning attribute changed will require a screenshot, but messages for which only the message meaning changed and which already have a screenshot do not require a new screenshot. And messages of which only the description attribute changed simply never require new screenshots at all. Bug: 1103844 Change-Id: I0318e685b2182a455dccfb0630ce768f5c990a65 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2412727 Reviewed-by: Jochen Eisinger <jochen@chromium.org> Reviewed-by: Mustafa Emre Acer <meacer@chromium.org> Commit-Queue: Rainhard Findling <rainhard@chromium.org> Cr-Commit-Position: refs/heads/master@{#808789}
This commit is contained in:

committed by
Commit Bot

parent
9b8ff423f5
commit
1a3e71ed39
19
PRESUBMIT.py
19
PRESUBMIT.py
@@ -5099,18 +5099,19 @@ def CheckStrings(input_api, output_api):
|
|||||||
removed_ids = old_ids - new_ids
|
removed_ids = old_ids - new_ids
|
||||||
modified_ids = set([])
|
modified_ids = set([])
|
||||||
for key in old_ids.intersection(new_ids):
|
for key in old_ids.intersection(new_ids):
|
||||||
if (old_id_to_msg_map[key].FormatXml()
|
if (old_id_to_msg_map[key].ContentsAsXml('', True)
|
||||||
!= new_id_to_msg_map[key].FormatXml()):
|
|
||||||
sha1_path = input_api.os_path.join(
|
|
||||||
screenshots_dir, key + '.png.sha1')
|
|
||||||
if sha1_path not in new_or_added_paths and \
|
|
||||||
not input_api.os_path.exists(sha1_path):
|
|
||||||
# This message does not yet have a screenshot. Require one.
|
|
||||||
modified_ids.add(key)
|
|
||||||
elif (old_id_to_msg_map[key].ContentsAsXml('', True)
|
|
||||||
!= new_id_to_msg_map[key].ContentsAsXml('', True)):
|
!= new_id_to_msg_map[key].ContentsAsXml('', True)):
|
||||||
# The message content itself changed. Require an updated screenshot.
|
# The message content itself changed. Require an updated screenshot.
|
||||||
modified_ids.add(key)
|
modified_ids.add(key)
|
||||||
|
elif old_id_to_msg_map[key].attrs['meaning'] != \
|
||||||
|
new_id_to_msg_map[key].attrs['meaning']:
|
||||||
|
# The message meaning changed. Ensure there is a screenshot for it.
|
||||||
|
sha1_path = input_api.os_path.join(screenshots_dir, key + '.png.sha1')
|
||||||
|
if sha1_path not in new_or_added_paths and not \
|
||||||
|
input_api.os_path.exists(sha1_path):
|
||||||
|
# There is neither a previous screenshot nor is a new one added now.
|
||||||
|
# Require a screenshot.
|
||||||
|
modified_ids.add(key)
|
||||||
|
|
||||||
if run_screenshot_check:
|
if run_screenshot_check:
|
||||||
# Check the screenshot directory for .png files. Warn if there is any.
|
# Check the screenshot directory for .png files. Warn if there is any.
|
||||||
|
@@ -2888,6 +2888,22 @@ class StringTest(unittest.TestCase):
|
|||||||
'</message>',
|
'</message>',
|
||||||
'</grit-part>')
|
'</grit-part>')
|
||||||
|
|
||||||
|
NEW_GRDP_CONTENTS5 = (
|
||||||
|
'<?xml version="1.0" encoding="utf-8"?>',
|
||||||
|
'<grit-part>',
|
||||||
|
'<message name="IDS_PART_TEST1" meaning="Meaning with typo.">',
|
||||||
|
'Part string 1',
|
||||||
|
'</message>',
|
||||||
|
'</grit-part>')
|
||||||
|
|
||||||
|
NEW_GRDP_CONTENTS6 = (
|
||||||
|
'<?xml version="1.0" encoding="utf-8"?>',
|
||||||
|
'<grit-part>',
|
||||||
|
'<message name="IDS_PART_TEST1" meaning="Meaning with typo fixed.">',
|
||||||
|
'Part string 1',
|
||||||
|
'</message>',
|
||||||
|
'</grit-part>')
|
||||||
|
|
||||||
# A grdp file with one ICU syntax message without syntax errors.
|
# A grdp file with one ICU syntax message without syntax errors.
|
||||||
NEW_GRDP_CONTENTS_ICU_SYNTAX_OK1 = (
|
NEW_GRDP_CONTENTS_ICU_SYNTAX_OK1 = (
|
||||||
'<?xml version="1.0" encoding="utf-8"?>',
|
'<?xml version="1.0" encoding="utf-8"?>',
|
||||||
@@ -2993,12 +3009,12 @@ class StringTest(unittest.TestCase):
|
|||||||
|
|
||||||
def testModifiedMessageDescription(self):
|
def testModifiedMessageDescription(self):
|
||||||
# CL modified a message description for a message that does not yet have a
|
# CL modified a message description for a message that does not yet have a
|
||||||
# screenshot. Should warn.
|
# screenshot. Should not warn.
|
||||||
input_api = self.makeInputApi([
|
input_api = self.makeInputApi([
|
||||||
MockAffectedFile('part.grdp', self.NEW_GRDP_CONTENTS3,
|
MockAffectedFile('part.grdp', self.NEW_GRDP_CONTENTS3,
|
||||||
self.NEW_GRDP_CONTENTS4, action='M')])
|
self.NEW_GRDP_CONTENTS4, action='M')])
|
||||||
warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
|
warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
|
||||||
self.assertEqual(1, len(warnings))
|
self.assertEqual(0, len(warnings))
|
||||||
|
|
||||||
# CL modified a message description for a message that already has a
|
# CL modified a message description for a message that already has a
|
||||||
# screenshot. Should not warn.
|
# screenshot. Should not warn.
|
||||||
@@ -3010,6 +3026,25 @@ class StringTest(unittest.TestCase):
|
|||||||
warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
|
warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
|
||||||
self.assertEqual(0, len(warnings))
|
self.assertEqual(0, len(warnings))
|
||||||
|
|
||||||
|
def testModifiedMessageMeaning(self):
|
||||||
|
# CL modified a message meaning for a message that does not yet have a
|
||||||
|
# screenshot. Should warn.
|
||||||
|
input_api = self.makeInputApi([
|
||||||
|
MockAffectedFile('part.grdp', self.NEW_GRDP_CONTENTS5,
|
||||||
|
self.NEW_GRDP_CONTENTS6, action='M')])
|
||||||
|
warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
|
||||||
|
self.assertEqual(1, len(warnings))
|
||||||
|
|
||||||
|
# CL modified a message meaning for a message that already has a
|
||||||
|
# screenshot. Should not warn.
|
||||||
|
input_api = self.makeInputApi([
|
||||||
|
MockAffectedFile('part.grdp', self.NEW_GRDP_CONTENTS5,
|
||||||
|
self.NEW_GRDP_CONTENTS6, action='M'),
|
||||||
|
MockFile(os.path.join('part_grdp', 'IDS_PART_TEST1.png.sha1'),
|
||||||
|
'binary', action='A')])
|
||||||
|
warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
|
||||||
|
self.assertEqual(0, len(warnings))
|
||||||
|
|
||||||
def testPngAddedSha1NotAdded(self):
|
def testPngAddedSha1NotAdded(self):
|
||||||
# CL added one new message in a grd file and added the png file associated
|
# CL added one new message in a grd file and added the png file associated
|
||||||
# with it, but did not add the corresponding sha1 file. This should warn
|
# with it, but did not add the corresponding sha1 file. This should warn
|
||||||
|
Reference in New Issue
Block a user