Skip to content

Commit

Permalink
[dpwa] Improve the test name convention for dpwa integration tests.
Browse files Browse the repository at this point in the history 
1. Use action names instead of action IDs in the test names.
2. Do not include the check actions in the test names.
3. Support a new column, which contains a shortened action name,  when reading actions.md.
4. Raise a RuntimeError when there is any test name that is > 256
   characters.
5. Add the shorten action names (in actions.md) for a few actions to
   ensure there are no tests with > 256 characters.
6. Use test steps to compare disabled tests instead of the name
   because test names only include state change actions.
7. Update all the unit tests and the test data for this change.

Bug: b/296244197
Change-Id: I6edd55d216fa846a29f5af5d7278646088e022b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4828638
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Clifford Cheng <cliffordcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1208872}
  • Loading branch information
Clifford Cheng authored and Chromium LUCI CQ committed Oct 12, 2023
1 parent 88e1d2b commit f7c30bf
Show file tree
Hide file tree
Showing 16 changed files with 315 additions and 99 deletions.
117 changes: 73 additions & 44 deletions chrome/test/webapps/file_reading.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@
from models import CoverageTestsByPlatformSet
from models import EnumsByType
from models import PartialAndFullCoverageByBaseName
from models import TestIdsByPlatform
from models import TestIdsByPlatformSet
from models import TestIdsTestNamesByPlatform
from models import TestIdsTestNamesByPlatformSet
from models import TestIdTestNameTuple
from models import TestPartitionDescription
from models import TestPlatform

Expand Down Expand Up @@ -291,22 +292,18 @@ def read_actions_file(
actions_by_name: Dict[str, Action] = {}
action_base_name_to_default_args: Dict[str, str] = {}
action_base_names: Set[str] = set()
all_ids: Set[str] = set()
for i, row in enumerate_markdown_file_lines_to_table_rows(
actions_file_lines):
if len(row) < MIN_COLUMNS_ACTIONS_FILE:
raise ValueError(f"Row {i!r} does not contain enough entries. "
f"Got {row}.")

shortened_base_name = row[7].strip() if len(row) > 7 else None
action_base_name = row[0].strip()
action_base_names.add(action_base_name)
if not re.fullmatch(r'[a-z_]+', action_base_name):
raise ValueError(f"Invald action base name {action_base_name} on "
f"row {i!r}. Please use snake_case.")
id_base = row[3].strip()
if not id_base or id_base in all_ids:
raise ValueError(f"Action id '{id_base}' on line {i!r} is "
f"not populated or already used.")

type = ActionType.STATE_CHANGE
if action_base_name.startswith("check_"):
Expand Down Expand Up @@ -358,7 +355,6 @@ def read_actions_file(

for arg_combination in all_arg_value_combinations:
name = "_".join([action_base_name] + arg_combination)
identifier = "".join([id_base] + arg_combination)

# If the action has arguments, then modify the output actions,
# and cpp method.
Expand Down Expand Up @@ -397,10 +393,9 @@ def read_actions_file(
raise ValueError(f"Cannot add duplicate action {name} on row "
f"{i!r}")

action = Action(name, action_base_name, identifier, cpp_method,
type, fully_supported_platforms,
action = Action(name, action_base_name, shortened_base_name,
cpp_method, type, fully_supported_platforms,
partially_supported_platforms)
all_ids.add(identifier)
action._output_canonical_action_names = (
output_canonical_action_names)
actions_by_name[action.name] = action
Expand Down Expand Up @@ -498,12 +493,13 @@ def read_unprocessed_coverage_tests_file(


def get_and_maybe_delete_tests_in_browsertest(
filename: str,
required_tests: Set[TestId] = {},
delete_in_place: bool = False) -> Dict[str, Set[TestPlatform]]:
filename: str,
required_tests: Set[TestIdTestNameTuple] = {},
delete_in_place: bool = False
) -> Dict[TestIdTestNameTuple, Set[TestPlatform]]:
"""
Returns a dictionary of all test ids found to the set of detected platforms
the test is enabled on.
Returns a dictionary of all test ids and test names found to
the set of detected platforms the test is enabled on.
When delete_in_place is set to True, overwrite the file to remove tests not
in required_tests.
Expand Down Expand Up @@ -532,60 +528,70 @@ def get_and_maybe_delete_tests_in_browsertest(
`TestPlatform.WINDOWS` and thus enabled on {`TestPlatform.MAC`,
`TestPlatform.CHROME_OS`, and `TestPlatform.LINUX`}.
"""
tests: Dict[str, Set[TestPlatform]] = {}
tests: Dict[TestIdTestNameTuple, Set[TestPlatform]] = {}

with open(filename, 'r') as fp:
file = fp.read()
result_file = file
# Attempts to match a full test case, where the name contains the test
# id prefix. Purposefully allows any prefixes on the test name (like
# MAYBE_ or DISABLED_).
# MAYBE_ or DISABLED_). Examples can be found here.
# https://regex101.com/r/l1xnAJ/2
for match in re.finditer(
fr'IN_PROC_BROWSER_TEST_F.+((?:\n.+)*)'
fr'{CoverageTest.TEST_ID_PREFIX}(\w+)\).+((?:\n.+)+)\n}}\n*',
file):
test_id = match.group(2)
tests[test_id] = set(TestPlatform)
test_name = f"{CoverageTest.TEST_ID_PREFIX}{test_id}"
if f"DISABLED_{test_name}" not in file:
if delete_in_place and test_id not in required_tests:
del tests[test_id]
'IN_PROC_BROWSER_TEST_F[\\(\\w\\s,]+'
fr'{CoverageTest.TEST_ID_PREFIX}([a-zA-Z0-9._-]+)\)'
'\\s*{\n(?:\\s*\\/\\/.*\n)+((?:[^;^}}]+;\n)+)}', file):
test_steps: List[str] = []
if match.group(2):
test_body = match.group(2).split("\n")
for line in test_body:
assert not line.strip().startswith("//")
test_steps.append(line.strip())
test_id = generate_test_id_from_test_steps(test_steps)
test_name = match.group(1)
tests[TestIdTestNameTuple(test_id, test_name)] = set(TestPlatform)
browser_test_name = f"{CoverageTest.TEST_ID_PREFIX}{test_name}"
if f"DISABLED_{browser_test_name}" not in file:
if delete_in_place and TestIdTestNameTuple(
test_id, test_name) not in required_tests:
del tests[TestIdTestNameTuple(test_id, test_name)]
# Remove the matching test code block when the test is not
# in required_tests
regex_to_remove = re.escape(match.group(0))
result_file = re.sub(regex_to_remove, '', result_file)
continue
enabled_platforms: Set[TestPlatform] = tests[test_id]
enabled_platforms: Set[TestPlatform] = tests[TestIdTestNameTuple(
test_id, test_name)]
for platform in TestPlatform:
# Search for macro that specifies the given platform before
# the string "DISABLED_<test_name>".
macro_for_regex = re.escape(platform.macro)
# This pattern ensures that there aren't any '{' or '}'
# characters between the macro and the disabled test name, which
# ensures that the macro is applying to the correct test.
if re.search(fr"{macro_for_regex}[^{{}}]+DISABLED_{test_name}",
file):
if re.search(
fr"{macro_for_regex}[^{{}}]+DISABLED_{browser_test_name}",
file):
enabled_platforms.remove(platform)
if len(enabled_platforms) == len(TestPlatform):
enabled_platforms.clear()
if delete_in_place:
with open(filename, 'w') as fp:
fp.write(result_file)

return tests


def find_existing_and_disabled_tests(
test_partitions: List[TestPartitionDescription],
required_coverage_by_platform_set: CoverageTestsByPlatformSet,
delete_in_place: bool = False
) -> Tuple[TestIdsByPlatformSet, TestIdsByPlatform]:
test_partitions: List[TestPartitionDescription],
required_coverage_by_platform_set: CoverageTestsByPlatformSet,
delete_in_place: bool = False
) -> Tuple[TestIdsTestNamesByPlatformSet, TestIdsTestNamesByPlatform]:
"""
Returns a dictionary of platform set to test id, and a dictionary of
platform to disabled test ids.
"""
existing_tests: TestIdsByPlatformSet = defaultdict(lambda: set())
disabled_tests: TestIdsByPlatform = defaultdict(lambda: set())
existing_tests: TestIdsNamesByPlatformSet = defaultdict(lambda: set())
disabled_tests: TestIdsNamesByPlatform = defaultdict(lambda: set())
for partition in test_partitions:
for file in os.listdir(partition.browsertest_dir):
if not file.startswith(partition.test_file_prefix):
Expand All @@ -594,18 +600,41 @@ def find_existing_and_disabled_tests(
TestPlatform.get_platforms_from_browsertest_filename(file))
filename = os.path.join(partition.browsertest_dir, file)
required_tests = set(
i.id
TestIdTestNameTuple(i.id, i.generate_test_name())
for i in required_coverage_by_platform_set.get(platforms, []))
tests = get_and_maybe_delete_tests_in_browsertest(
filename, required_tests, delete_in_place)
for test_id in tests.keys():
for test_id, test_name in tests.keys():
if test_id in existing_tests[platforms]:
raise ValueError(f"Already found test {test_id}. "
raise ValueError(f"Already found test {test_name}. "
f"Duplicate test in {filename}")
existing_tests[platforms].add(test_id)
existing_tests[platforms].add(
TestIdTestNameTuple(test_id, test_name))
for platform in platforms:
for test_id, enabled_platforms in tests.items():
for (test_id, test_name), enabled_platforms in tests.items():
if platform not in enabled_platforms:
disabled_tests[platform].add(test_id)
logging.info(f"Found tests in {filename}:\n{tests.keys()}")
disabled_tests[platform].add(
TestIdTestNameTuple(test_id, test_name))
test_names = [test_name for (test_id, test_name) in tests.keys()]
logging.info(f"Found tests in {filename}:\n{test_names}")
return (existing_tests, disabled_tests)


def generate_test_id_from_test_steps(test_steps: List[str]) -> str:
test_id = []
for test_step in test_steps:
# Examples of the matching regex.
# https://regex101.com/r/UYlzkK/1
match_test_step = re.search(r"helper_.(\w+)\(([\w,\s:]*)\);", test_step)
if match_test_step:
actions = re.findall('[A-Z][^A-Z]*', match_test_step.group(1))
test_id += [a.lower() for a in actions]
if match_test_step.group(2):
parameters = [
m.strip() for m in match_test_step.group(2).split(',')
]
for p in parameters:
match_param_value = re.match(r".*::k(.*)", p)
if match_param_value.group(1):
test_id.append(match_param_value.group(1))
return "_".join(test_id)
36 changes: 27 additions & 9 deletions chrome/test/webapps/file_reading_unittest.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from file_reading import expand_tests_from_action_parameter_wildcards
from file_reading import enumerate_markdown_file_lines_to_table_rows
from file_reading import human_friendly_name_to_canonical_action_name
from file_reading import generate_test_id_from_test_steps
from file_reading import get_and_maybe_delete_tests_in_browsertest
from file_reading import read_actions_file
from file_reading import read_enums_file
Expand All @@ -23,6 +24,7 @@
from models import ActionsByName
from models import ArgEnum
from models import CoverageTest
from models import TestIdTestNameTuple
from models import TestPlatform

TEST_DATA_DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)),
Expand Down Expand Up @@ -178,17 +180,17 @@ def test_coverage_file_reading(self):
coverage_tests = read_unprocessed_coverage_tests_file(
coverage_tsv, actions, enums,
action_base_name_to_default_param)

self.assertEqual(6, len(coverage_tests))

def test_browsertest_detection(self):
browsertest_filename = os.path.join(TEST_DATA_DIR, "tests_default.cc")
tests_and_platforms = get_and_maybe_delete_tests_in_browsertest(
browsertest_filename)
self.assertListEqual(list(tests_and_platforms.keys()),
["3Chicken_1Chicken_2ChickenGreen"])
tests_and_platforms = tests_and_platforms[
"3Chicken_1Chicken_2ChickenGreen"]
expected_key = TestIdTestNameTuple(
"state_change_a_Chicken_check_a_Chicken_check_b_Chicken_Green",
"3Chicken_1Chicken_2ChickenGreen")
self.assertListEqual(list(tests_and_platforms.keys()), [expected_key])
tests_and_platforms = tests_and_platforms[expected_key]
self.assertEqual(
{TestPlatform.LINUX, TestPlatform.CHROME_OS, TestPlatform.MAC},
tests_and_platforms)
Expand All @@ -200,20 +202,24 @@ def test_browertest_in_place_deletion(self):
output_file = os.path.join(tmpdirname, "output.cc")
shutil.copyfile(input_file, output_file)
tests_and_platforms = get_and_maybe_delete_tests_in_browsertest(
output_file, {"3Chicken_1Chicken_2ChickenGreen"},
output_file, {
TestIdTestNameTuple(
"state_change_a_Chicken_check_a_Chicken_check_b_Chicken_Green",
"StateChangeAChicken")
},
delete_in_place=True)

with open(output_file, 'r') as f, open(after_deletion_file,
'r') as f2:
self.assertTrue(f.read(), f2.read())

tests_and_platforms = tests_and_platforms[
"3Chicken_1Chicken_2ChickenGreen"]
tests_and_platforms = tests_and_platforms[TestIdTestNameTuple(
"state_change_a_Chicken_check_a_Chicken_check_b_Chicken_Green",
"3Chicken_1Chicken_2ChickenGreen")]
self.assertEqual(
{TestPlatform.LINUX, TestPlatform.CHROME_OS, TestPlatform.MAC},
tests_and_platforms)


def test_action_param_expansion(self):
enum_map: Dict[str, ArgEnum] = {
"EnumType": ArgEnum("EnumType", ["Value1", "Value2"], None)
Expand All @@ -234,6 +240,18 @@ def test_action_param_expansion(self):
['Action1(Value2)', 'Action2(Value2, Value2)']]
self.assertCountEqual(combinations, expected)

def test_generate_test_id_from_test_steps(self):
test_steps = [
"helper_.StateChangeA(Animal::kChicken);",
"helper_.CheckB(Animal::kChicken, Color::kGreen);",
"helper_.StateChangeB();"
]
test_id = generate_test_id_from_test_steps(test_steps)
expected_test_id = (
"state_change_a_Chicken_check_b_Chicken_Green_state_change_b"
)
self.assertEqual(test_id, expected_test_id)


if __name__ == '__main__':
unittest.main()
14 changes: 8 additions & 6 deletions chrome/test/webapps/generate_framework_tests_and_coverage.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,25 +135,27 @@ def generate_framework_tests_and_coverage(
all_partitions = [default_partition]
all_partitions.extend(custom_partitions)

(existing_tests_ids_by_platform_set,
disabled_test_ids_by_platform) = find_existing_and_disabled_tests(
(existing_tests_ids_names_by_platform_set,
disabled_test_ids_names_by_platform) = find_existing_and_disabled_tests(
all_partitions, required_coverage_by_platform_set, delete_in_place)

# Print all diffs that are required.
compare_and_print_tests_to_remove_and_add(
existing_tests_ids_by_platform_set, required_coverage_by_platform_set,
custom_partitions, default_partition, add_to_file)
existing_tests_ids_names_by_platform_set,
required_coverage_by_platform_set, custom_partitions,
default_partition, add_to_file)

if suppress_coverage:
return

# To calculate coverage we need to incorporate any disabled tests.
# Remove any disabled tests from the generated tests per platform.
for platform, tests in generated_tests_by_platform.items():
disabled_tests = disabled_test_ids_by_platform.get(platform, [])
disabled_tests = disabled_test_ids_names_by_platform.get(platform, [])
disabled_test_ids = set([test_id for (test_id, _) in disabled_tests])
tests_minus_disabled: List[CoverageTest] = []
for test in tests:
if test.id not in disabled_tests:
if test.id not in disabled_test_ids:
tests_minus_disabled.append(test)
else:
logging.info("Removing disabled test from coverage: " +
Expand Down

0 comments on commit f7c30bf

Please sign in to comment.