Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/14739.improvement.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
IdMaker.make_unique_parameterset_ids (used to generate ids for @pytest.mark.parametrize) no longer scales quadratically with the number of parameter sets when many of them produce colliding ids, significantly speeding up collection for large parametrized test suites with many duplicate ids.
11 changes: 9 additions & 2 deletions src/_pytest/python.py
Original file line number Diff line number Diff line change
Expand Up @@ -930,7 +930,8 @@ def make_unique_parameterset_ids(self) -> list[str | _HiddenParam]:
"""
resolved_ids = list(self._resolve_ids())
# All IDs must be unique!
if len(resolved_ids) != len(set(resolved_ids)):
all_ids = set(resolved_ids)
if len(resolved_ids) != len(all_ids):
# Record the number of occurrences of each ID.
id_counts = Counter(resolved_ids)

Expand Down Expand Up @@ -963,6 +964,8 @@ def make_unique_parameterset_ids(self) -> list[str | _HiddenParam]:
# Map the ID to its next suffix.
id_suffixes: dict[str, int] = defaultdict(int)
# Suffix non-unique IDs to make them unique.
used_ids = set(all_ids)
remaining_id_counts = id_counts.copy()
for index, id in enumerate(resolved_ids):
if id_counts[id] > 1:
if id is HIDDEN_PARAM:
Expand All @@ -971,10 +974,14 @@ def make_unique_parameterset_ids(self) -> list[str | _HiddenParam]:
if id and id[-1].isdigit():
suffix = "_"
new_id = f"{id}{suffix}{id_suffixes[id]}"
while new_id in set(resolved_ids):
while new_id in used_ids:
id_suffixes[id] += 1
new_id = f"{id}{suffix}{id_suffixes[id]}"
remaining_id_counts[id] -= 1
if remaining_id_counts[id] == 0:
used_ids.discard(id)
resolved_ids[index] = new_id
used_ids.add(new_id)
Comment on lines 983 to +984

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only half-updates the parallel set: resolved_ids[index] is replaced, but the old id is never removed from used_ids when it no longer occurs in the list.

That is not just an invariant smell — it changes assigned ids whenever a later candidate equals a fully-renamed-away earlier id.

Simplified counter-example (same algorithm as this loop):

# input
["a10", "a10"] + ["a"] * 11

# main today (rebuild set(resolved_ids) each probe)
["a10_0", "a10_1", "a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9", "a10"]

# this PR (stale "a10" left in used_ids)
["a10_0", "a10_1", "a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9", "a11"]  # <-- last id wrong

After both "a10" entries are renamed, main frees "a10" for the 11th "a". Here "a10" stays reserved forever, so the last id becomes "a11".

Please keep used_ids in sync, e.g. with a remaining-count:

used_ids = set(resolved_ids)
remaining = Counter(resolved_ids)
...
remaining[id] -= 1
if remaining[id] == 0:
    used_ids.discard(id)
resolved_ids[index] = new_id
used_ids.add(new_id)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, using a remaining_id_counts copy of id_counts decremented per rename, discarding once it hits zero. kinda the same idea as your sketch. Traced through the counter example of ["a10","a10"]+["a"]*11 and confirmed in the code that it correctly produces "a10" as the last id.

id_suffixes[id] += 1
assert len(resolved_ids) == len(set(resolved_ids)), (
f"Internal error: {resolved_ids=}"
Expand Down
33 changes: 33 additions & 0 deletions testing/python/metafunc.py
Original file line number Diff line number Diff line change
Expand Up @@ -785,6 +785,39 @@ def test_idmaker_duplicated_empty_str(self) -> None:
).make_unique_parameterset_ids()
assert result == ["0", "1"]

def test_idmaker_unique_ids_frees_renamed_away_ids(self) -> None:
"""Suffix collision must see ids freed after all duplicates are renamed.

Simplified resolved-id sequence: ["a10", "a10"] + ["a"] * 11
After renaming both "a10" entries, "a10" must be available again for
the 11th "a". A set that only .add()s new ids and never discards old
ones incorrectly yields "a11" as the last id.
"""
ids = ["a10", "a10"] + ["a"] * 11
result = IdMaker(
("x",),
[pytest.param(i) for i in range(len(ids))],
None,
ids,
None,
None,
).make_unique_parameterset_ids()
assert result == [
"a10_0",
"a10_1",
"a0",
"a1",
"a2",
"a3",
"a4",
"a5",
"a6",
"a7",
"a8",
"a9",
"a10",
]

def test_parametrize_ids_exception(self, pytester: Pytester) -> None:
"""
:param pytester: the instance of Pytester class, a temporary
Expand Down