Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1949353002:
Add ability to create CSV of Stale Test Expectations with list of all bugs and filenames (Closed)
|
Created:
4 years, 7 months ago by nainar Modified:
4 years, 7 months ago Reviewers:
Dirk Pranke CC:
blink-reviews, chromium-reviews, Dirk Pranke Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ability to create CSV of Stale Test Expectations with list of all bugs and filenames
This patch allows us to create a CSV of stale TestExpectations with
list of all bugs and filenames. A TestExpectations entry is considered
stale if the bug associated with it has not been updated in 90 days.
This patch also adds a fix to make sure you are taking user input into
consideration when checking how stale a bug is.
The design doc for this is here: https://drive.google.com/open?id=1oH67jAzE70-59JLoJ9r0NG--Ew7KL3Qtdh52D0Ogj7k
BUG=597797
Committed: https://crrev.com/41a56ae7ec64f52162ccd7a63f9f55596b84fae3
Cr-Commit-Position: refs/heads/master@{#393208}
Patch Set 1 #
Total comments: 8
Patch Set 2 : #
Total comments: 12
Patch Set 3 : #Patch Set 4 : Remove test #Messages
Total messages: 19 (7 generated)
nainar@chromium.org changed reviewers: + dpranke@chromium.org
Hi @dpranke! I have made a few changes to the print_stale_test_expectations_entries script so that it also generates a CSV. This will feed into another script that takes this CSV as an input and messages the individual bugs that are stale. Could you PTAL? https://codereview.chromium.org/1949353002/diff/1/test File test (right): https://codereview.chromium.org/1949353002/diff/1/test#newcode1 test:1: 501229,fast/forms/associatedFormControls-leak-nodes.html This is the CSV generated of bugs that have not been updated in 90 days - it is only here as a demo of the output - not for committing https://codereview.chromium.org/1949353002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries (right): https://codereview.chromium.org/1949353002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:48: class StaleTestCSV(object): I am creating an output csv right now - this seems like the best way as we move to automate this process
Description was changed from ========== Add ability to create csv of Stale Test Expectations with list of all bugs and filenames This patch allows us to create a CSV of stale TestExpectations with list of all bugs and filenames. A TestExpectations entry is considered stale if the bug associated with it has not been updated in 90 days. This patch also adds a fix to make sure you are taking user input into consideration when checking how stale a bug is BUG=597797 ========== to ========== Add ability to create CSV of Stale Test Expectations with list of all bugs and filenames This patch allows us to create a CSV of stale TestExpectations with list of all bugs and filenames. A TestExpectations entry is considered stale if the bug associated with it has not been updated in 90 days. This patch also adds a fix to make sure you are taking user input into consideration when checking how stale a bug is. The design doc for this is here: https://drive.google.com/open?id=1oH67jAzE70-59JLoJ9r0NG--Ew7KL3Qtdh52D0Ogj7k BUG=597797 ==========
https://codereview.chromium.org/1949353002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries (right): https://codereview.chromium.org/1949353002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:51: self._writer = csv.writer(open(filename, 'wb'), delimiter=',', quotechar='|', quoting=csv.QUOTE_MINIMAL, lineterminator='\n') using '|' as a quotechar seems like an odd choice. I'd either be inclined to just use the default excel dialect, or skip the csv module altogether since we control the format and just do file.write("%d,%s\n" % (bug_number, test_name)). https://codereview.chromium.org/1949353002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:68: return time_delta.days > (self._days if self._days else 90) you don't really need the if since self._days will default to 90. https://codereview.chromium.org/1949353002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:107: self._stale_test_csv.add_to_csv(bug_number, test_name) Ick. We shouldn't be parsing the file by hand. Do something like this instead: from webkitpy.common.host import Host from webkitpy.layout_tests.models.test_expectations import TestExpectationsParser port = Host().port_factory.get() exps = port.expectations_dict() parser = TestExpectationsParser(port, all_tests=True, is_lint_mode=False) for line in parser.parse(*exps.items()[0]): # The first item is the filename and contents of LayoutTests/TestExpectations bugs, name = line.bugs, line.name # there can be more than one bug# ! try: if bugs and all(self.is_stale(bug) for bug in bugs): print line.original_string.strip() if self._stale_test_csv._filename: self._stale_test_csv.add_to_csv(...) except: ...
Made the changes you suggested, PTAL? Thanks! https://codereview.chromium.org/1949353002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries (right): https://codereview.chromium.org/1949353002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:51: self._writer = csv.writer(open(filename, 'wb'), delimiter=',', quotechar='|', quoting=csv.QUOTE_MINIMAL, lineterminator='\n') Done. https://codereview.chromium.org/1949353002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:68: return time_delta.days > (self._days if self._days else 90) Done. https://codereview.chromium.org/1949353002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:107: self._stale_test_csv.add_to_csv(bug_number, test_name) Done.
https://codereview.chromium.org/1949353002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries (right): https://codereview.chromium.org/1949353002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:33: import csv this is no longer needed ... https://codereview.chromium.org/1949353002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:50: class StaleTestCSV(object): this class isn't really adding any value. I would suggest making the changes below, instead. https://codereview.chromium.org/1949353002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:61: self._stale_test_csv = StaleTestCSV(options.create_csv) change this to self.create_csv = options.create_csv. Also, there's no harm in making these fields public, really, so I'd remove the leading underscores. https://codereview.chromium.org/1949353002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:75: exps = port.expectations_dict() change this to : host = Host() port = host.port_factory.get() exps = port.expectations_dict() csv_contents = '' i.e., save the host pointer and add a new csv_contents var. https://codereview.chromium.org/1949353002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:83: self._stale_test_csv.add_to_csv(bugs[0], name) change lines 82-83 to: csv_contents += "%s, %s\n" % (bugs[0], name) This builds the csv_contents unconditionally, but that should be fine. https://codereview.chromium.org/1949353002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:91: print error add two lines de-dented out to match lines 76-77 (i.e., after the for loop) if self.create_csv: host.filesystem.write_text_file(self.create_csv, csv_contents)
PTAL? Thanks! https://codereview.chromium.org/1949353002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries (right): https://codereview.chromium.org/1949353002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:33: import csv Removed. https://codereview.chromium.org/1949353002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:50: class StaleTestCSV(object): Done. https://codereview.chromium.org/1949353002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:61: self._stale_test_csv = StaleTestCSV(options.create_csv) Done. Done. https://codereview.chromium.org/1949353002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:75: exps = port.expectations_dict() Done. https://codereview.chromium.org/1949353002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:83: self._stale_test_csv.add_to_csv(bugs[0], name) Done. https://codereview.chromium.org/1949353002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:91: print error Done.
lgtm, thanks!
The CQ bit was checked by nainar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949353002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949353002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Thanks committing after removing the test output
The CQ bit was checked by nainar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1949353002/#ps60001 (title: "Remove test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949353002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949353002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add ability to create CSV of Stale Test Expectations with list of all bugs and filenames This patch allows us to create a CSV of stale TestExpectations with list of all bugs and filenames. A TestExpectations entry is considered stale if the bug associated with it has not been updated in 90 days. This patch also adds a fix to make sure you are taking user input into consideration when checking how stale a bug is. The design doc for this is here: https://drive.google.com/open?id=1oH67jAzE70-59JLoJ9r0NG--Ew7KL3Qtdh52D0Ogj7k BUG=597797 ========== to ========== Add ability to create CSV of Stale Test Expectations with list of all bugs and filenames This patch allows us to create a CSV of stale TestExpectations with list of all bugs and filenames. A TestExpectations entry is considered stale if the bug associated with it has not been updated in 90 days. This patch also adds a fix to make sure you are taking user input into consideration when checking how stale a bug is. The design doc for this is here: https://drive.google.com/open?id=1oH67jAzE70-59JLoJ9r0NG--Ew7KL3Qtdh52D0Ogj7k BUG=597797 Committed: https://crrev.com/41a56ae7ec64f52162ccd7a63f9f55596b84fae3 Cr-Commit-Position: refs/heads/master@{#393208} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/41a56ae7ec64f52162ccd7a63f9f55596b84fae3 Cr-Commit-Position: refs/heads/master@{#393208} |
