[web-tests] Lint VirtualTestSuites for formatting [chromium/src : main]
Jiamei Liu (Gerrit)
Message from Jiamei Liu![]( "Open in Gerrit")
Set Ready For Review
Related details
- Jiamei Liu
- Jonathan Lee
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jiamei Liu (Gerrit)
Jiamei Liu voted and added 17 comments![]( "Open in Gerrit")
Votes added by Jiamei Liu
| Auto-Submit | +1 |
17 comments
Upload script to sort the VirtualTestSuites and sort the current VirtualTestSuites. Also added lint test against the VirtualTestSuites using the sorting script.Jiamei Liunit: Shorten the subject to 72 chars. Maybe:
```
[web-tests] Lint VirtualTestSuites for formatting
```You can explain below that:
- The autoformatter was added to make compliance with the linter easier
- The one-time reformat is needed to get this CL itself past CQ
Done
"""
Formats and sorts a JSON file containing a list of objects and string comments.Jiamei LiuFormatting nits from https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings:
- A one-line summary (shorten it until it fits <80 chars)
- Blank lines between the summary, extended description, the `Args` section, and the `Returns` section.
Done
json_content (str): The JSON content to format and sort.Jiamei Liu
- I don't think this is right. The tests are passing the parsed structure (i.e., a `list` of `str | dict`).
- Move type hints from the docstring to the signature. They're not actually checked yet, but it will be easier to start if they're there.
Done
do_sort (bool): Whether to sort the JSON content.Jiamei LiuI don't see the use case for not sorting. Let's just remove this parameter, always sort, and get rid of `_and_sort` from the function name (since formatting implies sorting).
Done
# --- Step 1: Find the first object and handle comment-only JSON ---
first_object_index = -1
for i, item in enumerate(json_content):
if isinstance(item, dict):
first_object_index = i
break
if first_object_index == -1:
return json.dumps(json_content, indent=2) + '\n'Jiamei LiuLet's get rid of this special case that handles a `VirtualTestSuites` file with no suites. Besides the unlikelihood of that happening, the general case should handle this if written correctly.
Done
# --- Step 2: Separate header comments from the main content ---
# Header comments are initial comments not immediately followed by an object.Jiamei LiuIIUC, this is trying to detect the preamble at the top of the file so sorting doesn't move it:
```
[
"<preamble>",
""
"<comment about first suite>" <-+ this block can move, but the preamble won't
{ |
"prefix": "first" |
}, <-+
]
```However, this breaks in a surprising way if the comments about the first suite span multiple lines:
```
[
"<preamble>",
""
"<comment about first suite>"
"<comment about first suite>"
"<comment about first suite>" <-+ comment lines can become separated
{ |
"prefix": "first" |
}, <-+
]
```I think the best way to make the boundary unambiguous is to simply introduce a magic `__BEGIN_SUITES__` comment.
We'll obviously want to test the second case too.
Done
if __name__ == '__main__':
unittest.main()
Jiamei Liunit: Unit tests in `blinkpy/` don't need to add this because the tests are run through `run_blinkpy_tests.py`.
Done
if expected_content is None:
failures.append('VirtualTestSuites is not a valid JSON file.')
return failuresJiamei LiuIsn't this dead code if `format_and_sort_json_with_comments()` always returns `str`?
Done
# Test a correctly formatted file.Jiamei Liunit: Make "unsorted suites", "incorrect indentation", and "correct" separate tests? See https://testing.googleblog.com/2018/06/testing-on-toilet-keep-tests-focused.html
Done
# Test a correctly formatted file.Jiamei Liunit: Make "unsorted suites", "incorrect indentation", and "correct" separate tests? See https://testing.googleblog.com/2018/06/testing-on-toilet-keep-tests-focused.html
Done
if __name__ == '__main__':Jiamei Liunit: Wrap everything below in `main()` so that we're not polluting the global namespace.
Done
# This script is intended to be run from the chromium/src checkout,
# e.g. with `./third_party/blink/tools/format_virtual_test_suites.py`Jiamei LiuThis seems unnecessarily restrictive. You can [use `PathFinder`][0]:
```
from blinkpy.common.host import Host
from blinkpy.common.path_finder import PathFinderhost = Host()
path_to_vts = PathFinder(host.filesystem).path_from_web_tests('VirtualTestSuites')
```(We ought to have a constant for `VirtualTestSuites` instead of copying the string everywhere)
Done
# If one argument is provided, it will format that file in-place.Jiamei Liunit: `VirtualTestSuites` should never move, so let's just assume `input_file = output_file = //third_party/blink/web_tests/VirtualTestSuites`.
Then there's no need to handle any arguments.
Done
data = NoneJiamei Liunit: Do we need this assignment? If there's an error, we'll always exit. If parsing `VirtualTestSuites` is successful, `data` will always be overwritten on L49.
Done
# To preserve empty lines, we convert them to empty string entries ""
# in the JSON array. An empty line is considered to be two or more
# newline characters between elements, possibly with whitespace.
# We look for a comma or an opening bracket, followed by whitespace
# that includes at least two newlines, and we insert an empty
# string "" before the next element.
content = re.sub(r'(?<=[,\[])(\s*\n){2,}\s*', '\n"",\n', content)Jiamei LiuI don't think we need this. Once we land this CL, it won't be possible to land new CLs that add completely blank lines to `VirtualTestSuites`, as the linter will detect a diff.
Done
data = json.loads(content)Jiamei Liunaming nit: WDYT about `entries`?
Done
except FileNotFoundError:
print(f"Error: Input file not found at '{input_file}'")
sys.exit(1)
except json.JSONDecodeError as e:
print(f"Error decoding JSON from '{input_file}': {e}")
print("Please ensure the file is a valid JSON array.")
sys.exit(1)Jiamei LiuLet's not catch these. Letting the error bubble to the top will provide a traceback instead of hiding it, and the error should be self-explanatory.
Done
Related details
- Jonathan Lee
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Lee (Gerrit)
Jonathan Lee added 8 comments![]( "Open in Gerrit")
"""
Formats and sorts a JSON file.
"""nit: IMO, we can remove this because the module only contains a single function that's already documented.
do_sort (bool): Whether to sort the JSON content.Jiamei LiuI don't see the use case for not sorting. Let's just remove this parameter, always sort, and get rid of `_and_sort` from the function name (since formatting implies sorting).
Done
Let's still remove `_and_sort` and simplify the name?
sortable_items.sort(key=lambda group: group[1].get('prefix', ''))`prefix` is a [required field][0], so we don't need to accommodate nonexistence here:
```suggestion
sortable_items.sort(key=lambda group: group[1]['prefix'])
```
Then we don't need the `test_missing_prefix` test.
final_sorted_list = []
final_sorted_list.extend(header_comments)nit: A direct copy could avoid a resize:
```suggestion
final_sorted_list = list(header_comments)
```
input_file = path_finder.path_from_web_tests('VirtualTestSuites')
output_file = input_filenit: Clean up into a single variable since they're the same now? Maybe `vts_path`.
print(f"Formatting '{input_file}' in-place.")
with open(input_file, 'r') as f:
entries = json.load(f)
sorted_json_content = format_and_sort_json_with_comments(entries)
with open(output_file, 'w') as f:
f.write(sorted_json_content)
print(
f"Successfully formatted '{input_file}' and saved to '{output_file}'")nit: The formatting should be pretty fast, so one final `print()` is probably enough:
```suggestion
with open(input_file, 'r') as f:
entries = json.load(f)
sorted_json_content = format_and_sort_json_with_comments(entries)
with open(output_file, 'w') as f:
f.write(sorted_json_content)
print(f"Successfully formatted '{input_file}'")
```
"This suite tests some android features on Linux and should never expire.",
"__BEGIN_SUITES__",L31 is a comment for the `android` suite; shouldn't `__BEGIN_SUITES__` go above it?
Related details
- Jiamei Liu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jiamei Liu (Gerrit)
Jiamei Liu voted and added 7 comments![]( "Open in Gerrit")
Votes added by Jiamei Liu
| Auto-Submit | +1 |
7 comments
Formats and sorts a JSON file.
"""nit: IMO, we can remove this because the module only contains a single function that's already documented.
Done
do_sort (bool): Whether to sort the JSON content.Jiamei LiuI don't see the use case for not sorting. Let's just remove this parameter, always sort, and get rid of `_and_sort` from the function name (since formatting implies sorting).
Jonathan LeeDone
Let's still remove `_and_sort` and simplify the name?
oops my bad, removed
sortable_items.sort(key=lambda group: group[1].get('prefix', ''))`prefix` is a [required field][0], so we don't need to accommodate nonexistence here:
```suggestion
sortable_items.sort(key=lambda group: group[1]['prefix'])
```Then we don't need the `test_missing_prefix` test.
Done
final_sorted_list = []
final_sorted_list.extend(header_comments)nit: A direct copy could avoid a resize:
```suggestion
final_sorted_list = list(header_comments)
```
Done
input_file = path_finder.path_from_web_tests('VirtualTestSuites')
output_file = input_filenit: Clean up into a single variable since they're the same now? Maybe `vts_path`.
Done
print(f"Formatting '{input_file}' in-place.")
with open(input_file, 'r') as f:
entries = json.load(f)
sorted_json_content = format_and_sort_json_with_comments(entries)
with open(output_file, 'w') as f:
f.write(sorted_json_content)
print(
f"Successfully formatted '{input_file}' and saved to '{output_file}'")nit: The formatting should be pretty fast, so one final `print()` is probably enough:
```suggestion
with open(input_file, 'r') as f:
entries = json.load(f)
sorted_json_content = format_and_sort_json_with_comments(entries)
with open(output_file, 'w') as f:
f.write(sorted_json_content)
print(f"Successfully formatted '{input_file}'")
```
Done
"This suite tests some android features on Linux and should never expire.",
"__BEGIN_SUITES__",L31 is a comment for the `android` suite; shouldn't `__BEGIN_SUITES__` go above it?
- Jonathan Lee
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Lee (Gerrit)
Jonathan Lee voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jonathan Lee
| Code-Review | +1 |
1 comment
+pdr@ for `VIRTUAL_OWNERS` (undoing the entropy that leaked back into `VirtualTestSuites` since https://crrev.com/c/6643672)
Related details
- Jiamei Liu
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
1 comment
third_party/blink/web_tests/VirtualTestSuites LGTM!
Related details
- Jiamei Liu
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jiamei Liu (Gerrit)
Jiamei Liu voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[web-tests] Lint VirtualTestSuites for formatting
1. Upload script to sort the VirtualTestSuites
2. Sort the current VirtualTestSuites.
3. Add lint test against the VirtualTestSuites
using the sorting script.
Script is from:
https://docs.google.com/document/d/13QRcFJaGywMJfOsxMNsfMwCG84rsLGQtF_YAxn154kg/edit?content_ref=sortable_items+sort+key+lambda+group+group+1+get+prefix&tab=t.0
- A third_party/blink/tools/blinkpy/style/virtual_suites_formatter.py
- A third_party/blink/tools/blinkpy/style/virtual_suites_formatter_unittest.py
- M third_party/blink/tools/blinkpy/web_tests/lint_test_expectations.py
- M third_party/blink/tools/blinkpy/web_tests/lint_test_expectations_unittest.py
- A third_party/blink/tools/format_virtual_test_suites.py
- M third_party/blink/web_tests/VirtualTestSuites
Code-Review: +1 by Philip Rogers, +1 by Jonathan Lee
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Lee (Gerrit)
Jonathan Lee has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |