|
|
Created:
6 years, 1 month ago by gayane -on leave until 09-2017 Modified:
6 years ago CC:
chromium-reviews, Ilya Sherman, arv+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionPresubmit checks for user actions introduced in HTML files.
This presubmit task will catch the case when HTML files are modified with new user action and action.xml is not updated
BUG=431393
Committed: https://crrev.com/3dff8c2676a1585b5945a8671f97d0b1e2eb1571
Cr-Commit-Position: refs/heads/master@{#306842}
Patch Set 1 #
Total comments: 8
Patch Set 2 : unit tests added #
Total comments: 22
Patch Set 3 : refactoring, better action search #
Total comments: 8
Patch Set 4 : comments added #
Total comments: 8
Patch Set 5 : comments fix. changed check for boolean actions. #
Total comments: 14
Patch Set 6 : nits and clean up #
Total comments: 7
Patch Set 7 : minor fixes #
Total comments: 4
Patch Set 8 : minor fixes #Patch Set 9 : actions.xml as a temporary file #
Total comments: 1
Patch Set 10 : comments for mock classes #
Messages
Total messages: 38 (9 generated)
gayane@chromium.org changed reviewers: + asvitkine@chromium.org, yiyaoliu@chromium.org
Please have a look at this initial draft.
https://codereview.chromium.org/719463003/diff/1/PRESUBMIT.py File PRESUBMIT.py (left): https://codereview.chromium.org/719463003/diff/1/PRESUBMIT.py#oldcode11 PRESUBMIT.py:11: Nit: Revert changes to this file. https://codereview.chromium.org/719463003/diff/1/chrome/browser/resources/PRE... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/1/chrome/browser/resources/PRE... chrome/browser/resources/PRESUBMIT.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. Nit: "Copyright 2014" (no (c)) https://codereview.chromium.org/719463003/diff/1/chrome/browser/resources/PRE... chrome/browser/resources/PRESUBMIT.py:20: action_re = r'metric\s*=\s*"([^ ]*)"' Should we check for whitespace before the start of this? e.g. this will match parametric= or some other prefixed thing which shouldn't trigger it. Is it possible to write tests for this presubmit? https://codereview.chromium.org/719463003/diff/1/tools/metrics/actions/extrac... File tools/metrics/actions/extract_actions.py (left): https://codereview.chromium.org/719463003/diff/1/tools/metrics/actions/extrac... tools/metrics/actions/extract_actions.py:766: Nit: Revert changes to this file.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Please review changes after added unittests and fixes for the minor issues. https://codereview.chromium.org/719463003/diff/1/PRESUBMIT.py File PRESUBMIT.py (left): https://codereview.chromium.org/719463003/diff/1/PRESUBMIT.py#oldcode11 PRESUBMIT.py:11: On 2014/11/11 20:36:41, Alexei Svitkine wrote: > Nit: Revert changes to this file. Done. https://codereview.chromium.org/719463003/diff/1/chrome/browser/resources/PRE... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/1/chrome/browser/resources/PRE... chrome/browser/resources/PRESUBMIT.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/11/11 20:36:41, Alexei Svitkine wrote: > Nit: "Copyright 2014" (no (c)) Done. https://codereview.chromium.org/719463003/diff/1/chrome/browser/resources/PRE... chrome/browser/resources/PRESUBMIT.py:20: action_re = r'metric\s*=\s*"([^ ]*)"' On 2014/11/11 20:36:41, Alexei Svitkine wrote: > Should we check for whitespace before the start of this? e.g. this will match > parametric= or some other prefixed thing which shouldn't trigger it. > > Is it possible to write tests for this presubmit? whitespaces or beginning of the line cases added to regex https://codereview.chromium.org/719463003/diff/1/tools/metrics/actions/extrac... File tools/metrics/actions/extract_actions.py (left): https://codereview.chromium.org/719463003/diff/1/tools/metrics/actions/extrac... tools/metrics/actions/extract_actions.py:766: On 2014/11/11 20:36:41, Alexei Svitkine wrote: > Nit: Revert changes to this file. I agree I should restore this line, but the other change is useful. Previously it would consider files without extension as well (e.g. OWNERS, LICENSES)
https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:4: """Presubmit script for HTML files in . Finish the sentence plz :) https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:40: action_disabled not in current_actions): I wonder how fast this is. Could you test? I remember they don't like it if we make presubmit a lot slower. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... File chrome/browser/resources/PRESUBMIT_test.py (right): https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT_test.py:41: 'metric="invalidaction" type="checkbox" dialog-pref>'] Could you test the case where "metrics=" is not at the start of the line?
https://codereview.chromium.org/719463003/diff/60001/PRESUBMIT_test.py File PRESUBMIT_test.py (right): https://codereview.chromium.org/719463003/diff/60001/PRESUBMIT_test.py#newcode15 PRESUBMIT_test.py:15: from PRESUBMIT_test_mockobjects import MockFile, MockInputApi, MockOutputApi,\ Nit: I prefer a slightly shorter name: PRESUBMIT_test_mocks.py https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:36: action_enabled = 'name="{0}_enabled"'.format(action_name) From extract_actions.py, it looks like the actual suffixes we want here are '_Enable' and '_Disable'. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:40: action_disabled not in current_actions): I think it would be cleaner to make a helper function for this, e.g. if not IsActionPresent(current_actions, action_name): return [output_api....] Then, in IsActionPresent, you can do the name formatting and this logic. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:47: Nit: Add an extra line. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... File chrome/browser/resources/PRESUBMIT_test.py (right): https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT_test.py:11: sys.path.append(os.path.abspath(os.path.dirname(__file__) + '/' +'../../../')) Nit: Remove the second + and just have those strings together. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT_test.py:31: PRESUBMIT.action_xml_path = _TEST_DATA_DIR + "actions.xml" Instead of overriding this global, how about making CheckUserActionUpdate take it as a param? Then, in the PRESUBMIT.py itself, you could actually make ACTIONS_XML_PATH a constant (upper case) and pass it to that function from the two CheckChange* methods. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT_test.py:48: Nit: Add an extra line. https://codereview.chromium.org/719463003/diff/60001/tools/metrics/actions/ex... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/719463003/diff/60001/tools/metrics/actions/ex... tools/metrics/actions/extract_actions.py:509: if ext and ext in extensions: This seems unrelated to the change you're making though. I suggest splitting to a separate CL.
A lot of new changes. - added different checks for boolean and non boolean actions. - changed the action search to be a bit better. - new tests https://codereview.chromium.org/719463003/diff/60001/PRESUBMIT_test.py File PRESUBMIT_test.py (right): https://codereview.chromium.org/719463003/diff/60001/PRESUBMIT_test.py#newcode15 PRESUBMIT_test.py:15: from PRESUBMIT_test_mockobjects import MockFile, MockInputApi, MockOutputApi,\ On 2014/11/12 22:27:00, Alexei Svitkine wrote: > Nit: I prefer a slightly shorter name: PRESUBMIT_test_mocks.py Done. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:4: """Presubmit script for HTML files in . On 2014/11/12 22:23:21, yao wrote: > Finish the sentence plz :) Done. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:36: action_enabled = 'name="{0}_enabled"'.format(action_name) On 2014/11/12 22:27:00, Alexei Svitkine wrote: > From extract_actions.py, it looks like the actual suffixes we want here are > '_Enable' and '_Disable'. Done. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:40: action_disabled not in current_actions): On 2014/11/12 22:27:00, Alexei Svitkine wrote: > I think it would be cleaner to make a helper function for this, e.g. > > if not IsActionPresent(current_actions, action_name): > return [output_api....] > > Then, in IsActionPresent, you can do the name formatting and this logic. Done. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:40: action_disabled not in current_actions): On 2014/11/12 22:23:21, yao wrote: > I wonder how fast this is. Could you test? > > I remember they don't like it if we make presubmit a lot slower. I tried to make it a little bit better. Now it will check whether the action should be boolean or not (which is necessary for correct check anyway). If it is not boolean then its just one pass over the actions.xml. If it is boolean then it first will search for {action_name}_Disabled and if found will search for {action_name}_Enabled continueing from the found place. Overall there will be one pass over actions.xml + few regex on the new_content to find out if it is boolean or not. I assume new content which has action defined wouldn't be usually that long. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:47: On 2014/11/12 22:27:00, Alexei Svitkine wrote: > Nit: Add an extra line. Done. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... File chrome/browser/resources/PRESUBMIT_test.py (right): https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT_test.py:11: sys.path.append(os.path.abspath(os.path.dirname(__file__) + '/' +'../../../')) On 2014/11/12 22:27:00, Alexei Svitkine wrote: > Nit: Remove the second + and just have those strings together. Done. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT_test.py:31: PRESUBMIT.action_xml_path = _TEST_DATA_DIR + "actions.xml" On 2014/11/12 22:27:00, Alexei Svitkine wrote: > Instead of overriding this global, how about making CheckUserActionUpdate take > it as a param? > > Then, in the PRESUBMIT.py itself, you could actually make ACTIONS_XML_PATH a > constant (upper case) and pass it to that function from the two CheckChange* > methods. Done. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT_test.py:41: 'metric="invalidaction" type="checkbox" dialog-pref>'] On 2014/11/12 22:23:21, yao wrote: > Could you test the case where "metrics=" is not at the start of the line? Done. https://codereview.chromium.org/719463003/diff/60001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT_test.py:48: On 2014/11/12 22:27:00, Alexei Svitkine wrote: > Nit: Add an extra line. Done. https://codereview.chromium.org/719463003/diff/60001/tools/metrics/actions/ex... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/719463003/diff/60001/tools/metrics/actions/ex... tools/metrics/actions/extract_actions.py:509: if ext and ext in extensions: On 2014/11/12 22:27:00, Alexei Svitkine wrote: > This seems unrelated to the change you're making though. I suggest splitting to > a separate CL. Done.
https://codereview.chromium.org/719463003/diff/80001/chrome/browser/resources... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/80001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:1: # Copyright (c) 2014 The Chromium Authors. All rights reserved. Nit: In new files, we don't include the (c) - please remove. https://codereview.chromium.org/719463003/diff/80001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:38: IsBoolean(f.NewContents(), action_name)): Nit: If you make is_boolean a local var above, does this if fit into one line? If so, I'd prefer that. https://codereview.chromium.org/719463003/diff/80001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:47: def IsActionPresent(current_actions, action_name, is_boolean): Please add some short comments above these methods that explain the params. https://codereview.chromium.org/719463003/diff/80001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:51: else: Nit: if there's an early return, remove the else and de-indent the remainder.
Done. Comments seem to big now. If feel like I can add or delete something let me know. https://codereview.chromium.org/719463003/diff/80001/chrome/browser/resources... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/80001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:1: # Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/11/15 00:15:47, Alexei Svitkine wrote: > Nit: In new files, we don't include the (c) - please remove. Done. https://codereview.chromium.org/719463003/diff/80001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:38: IsBoolean(f.NewContents(), action_name)): On 2014/11/15 00:15:47, Alexei Svitkine wrote: > Nit: If you make is_boolean a local var above, does this if fit into one line? > If so, I'd prefer that. Done. https://codereview.chromium.org/719463003/diff/80001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:47: def IsActionPresent(current_actions, action_name, is_boolean): On 2014/11/15 00:15:47, Alexei Svitkine wrote: > Please add some short comments above these methods that explain the params. Done. https://codereview.chromium.org/719463003/diff/80001/chrome/browser/resources... chrome/browser/resources/PRESUBMIT.py:51: else: On 2014/11/15 00:15:47, Alexei Svitkine wrote: > Nit: if there's an early return, remove the else and de-indent the remainder. Done.
https://codereview.chromium.org/719463003/diff/100001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/100001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:36: action_name = match.group(2) Hmm, maybe name this metric_name rather than action_name (to match the param it's being extracted from and the fact that it might not be the actual name of any action (due to suffixes). Please change throughout. https://codereview.chromium.org/719463003/diff/100001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:54: enabled one. You can make this shorter by describing the purpose of the function, rather than the implementation details. e.g. Checks whether there's matching entries in an actions.xml file for the given |action_name|, depending on whether it is a boolean action. https://codereview.chromium.org/719463003/diff/100001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:59: is_boolean: The identifer for the action type. What's an identifier for the action type? It would be clearer if you just say "Whether the action comes from a boolean control." https://codereview.chromium.org/719463003/diff/100001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:69: return pos >= 0 and current_actions.find(action_enabled, pos) >= 0 Why does "action in current_actions" work for the above case, but these need the .find() cases? Is it just an optimization so that you can pass in pos? Even if that's the intention, I don't think it's worth it. I don't think trying to optimize the case where there is an action in the CL, since that's more rare and is already going to run a bigger check.
https://codereview.chromium.org/719463003/diff/100001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/100001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:36: action_name = match.group(2) On 2014/11/15 01:07:48, Alexei Svitkine wrote: > Hmm, maybe name this metric_name rather than action_name (to match the param > it's being extracted from and the fact that it might not be the actual name of > any action (due to suffixes). Please change throughout. Done. https://codereview.chromium.org/719463003/diff/100001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:54: enabled one. On 2014/11/15 01:07:48, Alexei Svitkine wrote: > You can make this shorter by describing the purpose of the function, rather than > the implementation details. > > e.g. > > Checks whether there's matching entries in an actions.xml file for the given > |action_name|, depending on whether it is a boolean action. Done. https://codereview.chromium.org/719463003/diff/100001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:59: is_boolean: The identifer for the action type. On 2014/11/15 01:07:48, Alexei Svitkine wrote: > What's an identifier for the action type? It would be clearer if you just say > "Whether the action comes from a boolean control." Done. https://codereview.chromium.org/719463003/diff/100001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:69: return pos >= 0 and current_actions.find(action_enabled, pos) >= 0 On 2014/11/15 01:07:48, Alexei Svitkine wrote: > Why does "action in current_actions" work for the above case, but these need the > .find() cases? > > Is it just an optimization so that you can pass in pos? Even if that's the > intention, I don't think it's worth it. I don't think trying to optimize the > case where there is an action in the CL, since that's more rare and is already > going to run a bigger check. Done.
asvitkine@chromium.org changed reviewers: + maruel@chromium.org
LGTM, +maruel to review the presubmit bits
https://codereview.chromium.org/719463003/diff/120001/PRESUBMIT_test.py File PRESUBMIT_test.py (right): https://codereview.chromium.org/719463003/diff/120001/PRESUBMIT_test.py#newco... PRESUBMIT_test.py:15: from PRESUBMIT_test_mocks import MockFile, MockInputApi, MockOutputApi,\ use multiple from statements; from PRESUBMIT_test_mocks import MockChange, MockFile from PRESUBMIT_test_mocks import MockInputApi, MockOutputApi https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:11: import re remove https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:67: return action_disabled in current_actions and \ use () instead of \ everywhere (like line 84) https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:87: match = re.search(html_element_re, new_content) input_api.re. https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:91: if re.search(type_re, left_content) or re.search(type_re, right_content): return bool(re.search(type_re, left_content) or re.search(type_re, right_content)) or even simpler: return bool(re.search(type_re, match.group(i)) for i in (1, 3)) https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT_test.py (right): https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT_test.py:29: normally, 1 empty line between class methods https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT_test.py:35: self.assertEqual(0, len(warnings)) self.assertEqual([], self._testChange(lines)) would be more useful, as it would print out what failed in case of failure.
Done. https://codereview.chromium.org/719463003/diff/120001/PRESUBMIT_test.py File PRESUBMIT_test.py (right): https://codereview.chromium.org/719463003/diff/120001/PRESUBMIT_test.py#newco... PRESUBMIT_test.py:15: from PRESUBMIT_test_mocks import MockFile, MockInputApi, MockOutputApi,\ On 2014/11/26 18:15:48, M-A Ruel wrote: > use multiple from statements; > from PRESUBMIT_test_mocks import MockChange, MockFile > from PRESUBMIT_test_mocks import MockInputApi, MockOutputApi Done. https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:11: import re On 2014/11/26 18:15:48, M-A Ruel wrote: > remove Done. https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:67: return action_disabled in current_actions and \ On 2014/11/26 18:15:48, M-A Ruel wrote: > use () instead of \ > everywhere (like line 84) Done. https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:87: match = re.search(html_element_re, new_content) On 2014/11/26 18:15:48, M-A Ruel wrote: > input_api.re. Done. https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:91: if re.search(type_re, left_content) or re.search(type_re, right_content): On 2014/11/26 18:15:48, M-A Ruel wrote: > return bool(re.search(type_re, left_content) or re.search(type_re, > right_content)) > or even simpler: > return bool(re.search(type_re, match.group(i)) for i in (1, 3)) Done. https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT_test.py (right): https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT_test.py:29: On 2014/11/26 18:15:48, M-A Ruel wrote: > normally, 1 empty line between class methods Done. https://codereview.chromium.org/719463003/diff/120001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT_test.py:35: self.assertEqual(0, len(warnings)) On 2014/11/26 18:15:48, M-A Ruel wrote: > self.assertEqual([], self._testChange(lines)) > would be more useful, as it would print out what failed in case of failure. Done.
https://codereview.chromium.org/719463003/diff/140001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/140001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:87: any([input_api.re.search(type_re, match.group(i)) for i in (1, 3)])) remove [], they are unnecessary. https://codereview.chromium.org/719463003/diff/140001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT_test.py (right): https://codereview.chromium.org/719463003/diff/140001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT_test.py:11: sys.path.append(os.path.abspath(os.path.dirname(__file__) + '/../../../')) Normally, it's safer to do: os.path.dirname(os.path.abspath(__file__)) Then you'd loop 4 times. You may want to consider doing this instead of ../../../ due to windows path length limitation. https://codereview.chromium.org/719463003/diff/140001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT_test.py:28: self.assertEqual(0, len(warnings)) I meant for this one too.
https://codereview.chromium.org/719463003/diff/140001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/140001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:87: any([input_api.re.search(type_re, match.group(i)) for i in (1, 3)])) On 2014/11/26 20:43:27, M-A Ruel wrote: > remove [], they are unnecessary. Done. https://codereview.chromium.org/719463003/diff/140001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT_test.py (right): https://codereview.chromium.org/719463003/diff/140001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT_test.py:11: sys.path.append(os.path.abspath(os.path.dirname(__file__) + '/../../../')) On 2014/11/26 20:43:27, M-A Ruel wrote: > Normally, it's safer to do: > > os.path.dirname(os.path.abspath(__file__)) > > Then you'd loop 4 times. You may want to consider doing this instead of > ../../../ due to windows path length limitation. Do you mean to have something like this? sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))))) from PRESUBMIT_test_mocks import MockFile, MockInputApi, MockOutputApi https://codereview.chromium.org/719463003/diff/140001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT_test.py:28: self.assertEqual(0, len(warnings)) On 2014/11/26 20:43:27, M-A Ruel wrote: > I meant for this one too. Done.
lgtm https://codereview.chromium.org/719463003/diff/140001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT_test.py (right): https://codereview.chromium.org/719463003/diff/140001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT_test.py:11: sys.path.append(os.path.abspath(os.path.dirname(__file__) + '/../../../')) On 2014/11/26 21:57:28, gayane wrote: > On 2014/11/26 20:43:27, M-A Ruel wrote: > > Normally, it's safer to do: > > > > os.path.dirname(os.path.abspath(__file__)) > > > > Then you'd loop 4 times. You may want to consider doing this instead of > > ../../../ due to windows path length limitation. > > Do you mean to have something like this? > sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))))) > from PRESUBMIT_test_mocks import MockFile, MockInputApi, MockOutputApi Yes but if it works, it's fine with me. https://codereview.chromium.org/719463003/diff/160001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/160001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:21: file_filter = lambda f: f.LocalPath().endswith(('.html')) file_filter = lambda f: f.LocalPath().endswith('.html') https://codereview.chromium.org/719463003/diff/160001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT_test.py (right): https://codereview.chromium.org/719463003/diff/160001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT_test.py:72: MockOutputApi(), alignment. That's one of the reason I always use +4 instead.
https://codereview.chromium.org/719463003/diff/160001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT.py (right): https://codereview.chromium.org/719463003/diff/160001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:21: file_filter = lambda f: f.LocalPath().endswith(('.html')) On 2014/11/28 17:34:24, M-A Ruel wrote: > file_filter = lambda f: f.LocalPath().endswith('.html') Done. https://codereview.chromium.org/719463003/diff/160001/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT_test.py (right): https://codereview.chromium.org/719463003/diff/160001/chrome/browser/resource... chrome/browser/resources/PRESUBMIT_test.py:72: MockOutputApi(), On 2014/11/28 17:34:25, M-A Ruel wrote: > alignment. That's one of the reason I always use +4 instead. Done.
gayane@chromium.org changed reviewers: + nkostylev@chromium.org, phajdan.jr@chromium.org
nkostylev@chromium.org: Please owner's review for changes in chrome/browser/resources/* phajdan.jr@chromium.org: Please owner's review for PRESUBMIT_test.py PRESUBMIT_test_mocks.py base/test/data/presubmit/actions.xml
phajdan.jr@chromium.org changed reviewers: + brettw@chromium.org
+brettw for base/test/data I think we historically avoided adding more things there unless necessary. Brett, is that still true, and if so, what's your recommendation?
It would be better if this file was by the test rather than in a completely unrelated toplevel module. Even better might be to write it to a temp dir and just keep the data in the python file since it's so small.
gayane@chromium.org changed reviewers: - nkostylev@chromium.org
Removed actions.xml file. Now creating it as a temporary file.
LGTM
On 2014/12/03 10:51:17, Paweł Hajdan Jr. wrote: > LGTM brettw@ do you think you can have a look at the rest of the CL? chrome/browser/resources/PRESUBMIT.py and chrome/browser/resources/PRESUBMIT_test.py still needs owners approval.
LGTM with comment. https://codereview.chromium.org/719463003/diff/200001/PRESUBMIT_test_mocks.py File PRESUBMIT_test_mocks.py (right): https://codereview.chromium.org/719463003/diff/200001/PRESUBMIT_test_mocks.py... PRESUBMIT_test_mocks.py:12: class MockInputApi(object): Can these classes have class-level comments explaining briefly how to use the,? Now they're in a separate file they have more of a "public interface" feel and should be more friendly to people wanting to use them.
Patchset #10 (id:220001) has been deleted
On 2014/12/03 19:28:02, brettw wrote: > LGTM with comment. > > https://codereview.chromium.org/719463003/diff/200001/PRESUBMIT_test_mocks.py > File PRESUBMIT_test_mocks.py (right): > > https://codereview.chromium.org/719463003/diff/200001/PRESUBMIT_test_mocks.py... > PRESUBMIT_test_mocks.py:12: class MockInputApi(object): > Can these classes have class-level comments explaining briefly how to use the,? > Now they're in a separate file they have more of a "public interface" feel and > should be more friendly to people wanting to use them. Class description comments added for the mock classes.
The CQ bit was checked by gayane@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/719463003/240001
Message was sent while issue was closed.
Committed patchset #10 (id:240001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3dff8c2676a1585b5945a8671f97d0b1e2eb1571 Cr-Commit-Position: refs/heads/master@{#306842}
Message was sent while issue was closed.
On 2014/12/04 17:11:25, I haz the power (commit-bot) wrote: > Patchset 10 (id:??) landed as > https://crrev.com/3dff8c2676a1585b5945a8671f97d0b1e2eb1571 > Cr-Commit-Position: refs/heads/master@{#306842} This is preventing me from commiting a change that seems valid, or uploading a change that seems valid. https://codereview.chromium.org/747883002/ |