Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2674553002:
MD WebUI: attempt to clean up file paths on vulcanize+Windows (Closed)
DescriptionMD WebUI: attempt to clean up file paths on vulcanize+Windows
BUG=673825
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2674553002
Cr-Commit-Position: refs/heads/master@{#448659}
Committed: https://chromium.googlesource.com/chromium/src/+/49fb7c71e261bad101ed81e873f57ee3ec11fbb5
Patch Set 1 #
Total comments: 3
Patch Set 2 : not #Patch Set 3 : tests #
Total comments: 7
Patch Set 4 : depfile, +x #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 25 (15 generated)
Description was changed from ========== MD WebUI: attempt to clean up the file path shitshow on vulcanize+Windows BUG=673825 R=dpapad@chromium.org ========== to ========== MD WebUI: attempt to clean up the file path shitshow on vulcanize+Windows BUG=673825 R=dpapad@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2674553002/diff/1/chrome/browser/resources/vu... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2674553002/diff/1/chrome/browser/resources/vu... chrome/browser/resources/vulcanize_gn.py:141: deps = [d for d in deps if d.startswith(filter_url)] I think this is missing a "not" deps = [d for d in deps if not d.startswith(filter_url)]
https://codereview.chromium.org/2674553002/diff/1/chrome/browser/resources/vu... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2674553002/diff/1/chrome/browser/resources/vu... chrome/browser/resources/vulcanize_gn.py:141: deps = [d for d in deps if d.startswith(filter_url)] On 2017/02/02 at 01:39:37, dpapad wrote: > I think this is missing a "not" > > deps = [d for d in deps if not d.startswith(filter_url)] Verified locally (on Linux). This was causing the wrong files to be filtered out, and consequently the wrong files to be showing up in the depfile.
dpapad@chromium.org changed reviewers: - dpapad@chromium.org
not verified on windows, yet https://codereview.chromium.org/2674553002/diff/1/chrome/browser/resources/vu... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2674553002/diff/1/chrome/browser/resources/vu... chrome/browser/resources/vulcanize_gn.py:141: deps = [d for d in deps if d.startswith(filter_url)] On 2017/02/02 01:43:03, dpapad wrote: > On 2017/02/02 at 01:39:37, dpapad wrote: > > I think this is missing a "not" > > > > deps = [d for d in deps if not d.startswith(filter_url)] > > Verified locally (on Linux). This was causing the wrong files to be filtered > out, and consequently the wrong files to be showing up in the depfile. Done.
Description was changed from ========== MD WebUI: attempt to clean up the file path shitshow on vulcanize+Windows BUG=673825 R=dpapad@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD WebUI: attempt to clean up file paths on vulcanize+Windows BUG=673825 R=dpapad@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: + dpapad@chromium.org
ptal
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2674553002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2674553002/diff/40001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn.py:204: parser.add_argument('--depfile') This is required too. If it needs to be optional temporarily, until we add tests for that codepath too, let's state that explicitly, in a comment. https://codereview.chromium.org/2674553002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vulcanize_gn_test.py (right): https://codereview.chromium.org/2674553002/diff/40001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn_test.py:56: <link rel="import" href="element.html"> That's a good start. I think it would be valuable to test the effect of the --redirect flags. Also I think instead of making specific toy assertions in the generated file, a much more common pattern I have encountered (more than once) for testing the code generators is the following: 1) Use dummy input files (element.html, element.js, ui.html). 2) Run the tool being tested, and commit the output as the ground truth. 3) When asserting, just assert the entire contents of the file match the ground truth. 4) When making changes to the generation script, need to update the ground truth (after manually inspecting that the diff between previous truth and new one is correct). https://codereview.chromium.org/2674553002/diff/40001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn_test.py:64: fast_html_css = self._read_out_file('fast.html') I am confused by the '_css' suffix. Aren't we just reading the contents of the generated HTML file here? If so why not naming this |fast_html| or |fast_html_contents|?
https://codereview.chromium.org/2674553002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2674553002/diff/40001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn.py:204: parser.add_argument('--depfile') On 2017/02/07 03:00:32, dpapad wrote: > This is required too. If it needs to be optional temporarily, until we add tests > for that codepath too, let's state that explicitly, in a comment. made required https://codereview.chromium.org/2674553002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vulcanize_gn_test.py (right): https://codereview.chromium.org/2674553002/diff/40001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn_test.py:56: <link rel="import" href="element.html"> On 2017/02/07 03:00:32, dpapad wrote: > That's a good start. I think it would be valuable to test the effect of the > --redirect flags. Also I think instead of making specific toy assertions in the > generated file, a much more common pattern I have encountered (more than once) > for testing the code generators is the following: > > 1) Use dummy input files (element.html, element.js, ui.html). > 2) Run the tool being tested, and commit the output as the ground truth. > 3) When asserting, just assert the entire contents of the file match the ground > truth. > 4) When making changes to the generation script, need to update the ground > truth (after > manually inspecting that the diff between previous truth and new one is > correct). #patcheswelcome, I named this "simple" for a reason https://codereview.chromium.org/2674553002/diff/40001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn_test.py:64: fast_html_css = self._read_out_file('fast.html') On 2017/02/07 03:00:32, dpapad wrote: > I am confused by the '_css' suffix. Aren't we just reading the contents of the > generated HTML file here? If so why not naming this |fast_html| or > |fast_html_contents|? fixed in updated patch
LGTM https://codereview.chromium.org/2674553002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vulcanize_gn_test.py (right): https://codereview.chromium.org/2674553002/diff/40001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn_test.py:56: <link rel="import" href="element.html"> On 2017/02/07 at 03:02:39, Dan Beam wrote: > On 2017/02/07 03:00:32, dpapad wrote: > > That's a good start. I think it would be valuable to test the effect of the > > --redirect flags. Also I think instead of making specific toy assertions in the > > generated file, a much more common pattern I have encountered (more than once) > > for testing the code generators is the following: > > > > 1) Use dummy input files (element.html, element.js, ui.html). > > 2) Run the tool being tested, and commit the output as the ground truth. > > 3) When asserting, just assert the entire contents of the file match the ground > > truth. > > 4) When making changes to the generation script, need to update the ground > > truth (after > > manually inspecting that the diff between previous truth and new one is > > correct). > > #patcheswelcome, I named this "simple" for a reason I was not implying to do this in this CL. Mostly I wanted to see WDYT about that approach. I am interpreting #patcheswelcome as a "SGTM". https://codereview.chromium.org/2674553002/diff/60001/chrome/browser/resource... File chrome/browser/resources/vulcanize_gn_test.py (right): https://codereview.chromium.org/2674553002/diff/60001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn_test.py:9: import unittest Is vulcanize_gn_test.py picked up automatically by some script, or do we need to register it somewhere? (I have never been involved in python test infrastructure before).
https://codereview.chromium.org/2674553002/diff/60001/chrome/browser/resource... File chrome/browser/resources/vulcanize_gn_test.py (right): https://codereview.chromium.org/2674553002/diff/60001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn_test.py:9: import unittest On 2017/02/07 03:44:22, dpapad wrote: > Is vulcanize_gn_test.py picked up automatically by some script, or do we need to > register it somewhere? (I have never been involved in python test infrastructure > before). currently manual, we'd need to make it automated
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486489252220790,
"parent_rev": "40937c0272cf7ae2342b318cf394dbdaddaa5a76", "commit_rev":
"49fb7c71e261bad101ed81e873f57ee3ec11fbb5"}
Message was sent while issue was closed.
Description was changed from ========== MD WebUI: attempt to clean up file paths on vulcanize+Windows BUG=673825 R=dpapad@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD WebUI: attempt to clean up file paths on vulcanize+Windows BUG=673825 R=dpapad@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2674553002 Cr-Commit-Position: refs/heads/master@{#448659} Committed: https://chromium.googlesource.com/chromium/src/+/49fb7c71e261bad101ed81e873f5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/49fb7c71e261bad101ed81e873f5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
