Skip to content

[uss_qualifier/reports] Typing fixes #1385

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
the-glu wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[uss_qualifier/reports] Typing fixes #1385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
766 changes: 0 additions & 766 deletions .basedpyright/baseline.json
View file
Open in desktop

Large diffs are not rendered by default.

53 changes: 30 additions & 23 deletions monitoring/uss_qualifier/reports/globally_expanded/generate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ def generate_globally_expanded_report(

"""

assert report.configuration.v1 and report.configuration.v1.test_run
resource_pool = make_resources_config(report.configuration.v1.test_run)

def indented_ul(value) -> list[str]:
Expand Down Expand Up @@ -184,6 +185,7 @@ def describe_pool_resource(k: str, v: dict) -> str:

def _generate_sections(node: ActionNode) -> Iterator[_Section]:
if node.node_type == ActionNodeType.Scenario:
assert node.scenario
yield _generate_scenario_section(node.scenario)
elif node.node_type == ActionNodeType.SkippedAction:
yield _generate_skipped_scenario_section(node)
Expand All @@ -195,7 +197,7 @@ def _generate_sections(node: ActionNode) -> Iterator[_Section]:
def _generate_skipped_scenario_section(node: ActionNode) -> _Section:
return _Section(
title=f"[skipped] {node.name}",
body=f"This instance of this test scenario was skipped in this test run because: {node.skipped_action.reason}",
body=f"This instance of this test scenario was skipped in this test run because: {node.skipped_action.reason if node.skipped_action else '?'}",
)


Expand Down Expand Up @@ -237,15 +239,15 @@ def _indent_headings(elements: Sequence[marko.element.Element], levels: int) ->
for element in elements:
if isinstance(element, marko.block.Heading):
element.level = min(element.level + levels, 6)
if hasattr(element, "children") and element.children:
_indent_headings(element.children, levels)
if children := getattr(element, "children", None):
Copy link
Member

@BenjaminPelletier BenjaminPelletier Mar 9, 2026

Choose a reason for hiding this comment

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

The problem with the approach is that IDEs don't generally seem to find fields by getattr string-literal usage. So, if someone was looking at the field children in the type of object that element is, they would find an instance of that field's usage with the previous approach, but they wouldn't find an instance of that field's usage with this change.

I'm not strongly against changing between two equally-valid approaches if basedpyright prefers one of them (although that is still a use of limited and valuable dev time that could be applied elsewhere), but we certainly shouldn't let basedpyright cause us to switch from an approach with more advantages to an approach with less advantages.

Copy link
Member

@BenjaminPelletier BenjaminPelletier Mar 11, 2026

Choose a reason for hiding this comment

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

Unlike the comment below, I think we do want to keep the original here to expose the use of the children attribute to IDEs. So, I think #pyright: ignore is appropriate here instead.

_indent_headings(children, levels)


def _inflate_fragments(parent: marko.element.Element, origin_filename: str) -> None:
if hasattr(parent, "children") and parent.children:
if hasattr(parent, "children") and parent.children: # pyright: ignore[reportAttributeAccessIssue]
c = 0
while c < len(parent.children):
child = parent.children[c]
while c < len(parent.children): # pyright: ignore[reportAttributeAccessIssue]
child = parent.children[c] # pyright: ignore[reportAttributeAccessIssue]
if (
isinstance(child, marko.block.Heading)
and hasattr(child, "children")
Expand All @@ -260,12 +262,12 @@ def _inflate_fragments(parent: marko.element.Element, origin_filename: str) -> N
doc = _get_test_step_fragment(absolute_path, child.level)
_update_links(doc, absolute_path)
_strip_link(child)
parent.children = (
parent.children[0 : c + 1] + doc.children + parent.children[c + 1 :]
parent.children = ( # pyright: ignore[reportAttributeAccessIssue]
parent.children[0 : c + 1] + doc.children + parent.children[c + 1 :] # pyright: ignore[reportAttributeAccessIssue]
)
c += len(doc.children)
elif isinstance(child, marko.element.Element):
_inflate_fragments(parent.children[c], origin_filename)
_inflate_fragments(parent.children[c], origin_filename) # pyright: ignore[reportAttributeAccessIssue]
c += 1
else:
c += 1
Expand All @@ -286,14 +288,14 @@ def add_resource_origin():
note = marko.parse(
"""&empty; _This resource was not applicable to this test run and was therefore not provided._\n\n"""
)
doc.children = doc.children[0:c] + note.children + doc.children[c:]
doc.children = doc.children[0:c] + note.children + doc.children[c:] # pyright: ignore[reportAttributeAccessIssue,reportOperatorIssue]
c += len(note.children)
return
# Insert resource origin information
origin = marko.parse(
f"\n\n&#x2705; Provided by {scenario.resource_origins[current_resource]}.\n"
)
doc.children = doc.children[0:c] + origin.children + doc.children[c:]
doc.children = doc.children[0:c] + origin.children + doc.children[c:] # pyright: ignore[reportOperatorIssue]
c += len(origin.children)

while c < len(doc.children):
Expand Down Expand Up @@ -327,11 +329,11 @@ def add_resource_origin():


def _strip_link(element: marko.element.Element) -> None:
if hasattr(element, "children") and element.children:
for c in range(len(element.children)):
child = element.children[c]
if hasattr(element, "children") and element.children: # pyright: ignore[reportAttributeAccessIssue]
for c in range(len(element.children)): # pyright: ignore[reportAttributeAccessIssue]
child = element.children[c] # pyright: ignore[reportAttributeAccessIssue]
if isinstance(child, marko.block.inline.Link):
element.children[c] = child.children[0]
element.children[c] = child.children[0] # pyright: ignore[reportAttributeAccessIssue]
elif isinstance(child, marko.element.Element):
_strip_link(child)

Expand Down Expand Up @@ -372,7 +374,7 @@ def add_context_to_case():
"""&empty; _This test case was not applicable to this test run and is therefore not statused._\n\n"""
)
doc.children = (
doc.children[0 : test_case_i0 + 1]
doc.children[0 : test_case_i0 + 1] # pyright: ignore[reportAttributeAccessIssue,reportOperatorIssue]
+ note.children
+ doc.children[test_case_i0 + 1 :]
)
Expand All @@ -396,6 +398,7 @@ def add_context_to_case():
for epoch in scenario.epochs:
if epoch.type != EpochType.Case:
continue
assert epoch.case
Copy link
Member

@BenjaminPelletier BenjaminPelletier Mar 9, 2026

Choose a reason for hiding this comment

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

The epoch.type check above ensures epoch.case is defined, so I don't think this redundant check is an improvement. The pattern where an object can have one of multiple child payload types (Case or Events in this case; test_scenario, test_suite, action_generator for TestSuiteAction; etc) recurs in this codebase so it seems like we should have a standard approach. Perhaps the type property accessor (or similar) should never be used to switch between behaviors, and instead the fields should be inspected directly? E.g.:

for epoch in scenario.epochs:
    if not epoch.case:
        continue
    if case_name == ...

Copy link
Contributor Author

@the-glu the-glu Mar 10, 2026

Choose a reason for hiding this comment

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

I do agree we have it a lot and we should switch to use the fields directly, but this there is one catch: is it safe, through the code base so assume those fields uniquely define the thing we should do? Or in others words, is only one of (test_scenario, test_suite, action_generator) not None?

Copy link
Member

@BenjaminPelletier BenjaminPelletier Mar 11, 2026

Choose a reason for hiding this comment

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

is it safe, through the code base so assume those fields uniquely define the thing we should do? Or in others words, is only one of (test_scenario, test_suite, action_generator) not None?

That's the intent in these places -- basically equivalent to oneof. We should certainly clarify that through documentation at least anywhere that's unclear. Actual annotations would be awesome, but I don't know of any way to do that in Python. My documentation pattern would probably look something like:

class Epoch(ImplicitDict):
    # === OneOf: Only one of these properties may be specified ===
    case: Optional[TestedCase] = None
    events: Optional[list[Event]] = None
    # === End OneOf ===

The giveaway that this is the case when looking at existing code is something that implies only one may be set; e.g.:

    @property
    def type(self) -> EpochType:
        if self.case:
            return EpochType.Case
        elif self.events:
            return EpochType.Events
        else:
            raise ValueError("Invalid Epoch did not specify case or events")

if case_name == epoch.case.name:
test_case = epoch.case
break
Expand All @@ -409,6 +412,7 @@ def add_context_to_case():
for epoch in scenario.epochs:
if epoch.type != EpochType.Case:
continue
assert epoch.case
if len(epoch.case.steps) == 1 and epoch.case.steps[0].name == "Cleanup":
test_case = epoch.case
break
Expand Down Expand Up @@ -444,7 +448,7 @@ def add_context_to_step():
)
dc = len(note.children)
doc.children = (
doc.children[0 : test_step_i0 + 1]
doc.children[0 : test_step_i0 + 1] # pyright: ignore[reportOperatorIssue]
+ note.children
+ doc.children[test_step_i0 + 1 :]
)
Expand Down Expand Up @@ -494,6 +498,7 @@ def _add_context_to_step(
def add_context_to_check():
nonlocal c, i1, added, test_check_name, test_check_i0, test_check_level
if test_check_name is not None:
assert test_check_i0 is not None
dc = _add_context_to_check(doc, step, test_check_name, test_check_i0, c)
c += dc
i1 += dc
Expand Down Expand Up @@ -534,13 +539,15 @@ def _add_context_to_check(
for event in step.events:
if (
event.type == EventType.PassedCheck
and event.passed_check is not None
and event.passed_check.name == test_check_name
):
check_text.append(
f"&#x2705; {', '.join(event.passed_check.participants)} ({event.passed_check.timestamp})"
)
elif (
event.type == EventType.FailedCheck
and event.failed_check
and event.failed_check.name == test_check_name
):
check_text.append(
Expand All @@ -552,7 +559,7 @@ def _add_context_to_check(
additions = marko.parse(
"""&empty; _This check was not applicable to this test run and is therefore not statused._\n\n"""
)
doc.children = doc.children[0:i1] + additions.children + doc.children[i1:]
doc.children = doc.children[0:i1] + additions.children + doc.children[i1:] # pyright: ignore[reportOperatorIssue]
return len(additions.children)


Expand All @@ -571,16 +578,16 @@ def _update_links(element: marko.element.Element, origin_filename: str) -> None:
url = url.replace("/github.com/", "/raw.githubusercontent.com/")
url = url.replace("/blob/", "/")
element.dest = url
if hasattr(element, "children") and element.children:
for child in element.children:
if hasattr(element, "children") and element.children: # pyright: ignore[reportAttributeAccessIssue]
for child in element.children: # pyright: ignore[reportAttributeAccessIssue]
if isinstance(child, marko.element.Element):
_update_links(child, origin_filename)


def _add_section_numbers(elements: Sequence[marko.element.Element]) -> None:
heading_level = 2
levels = [0]
headings = [None]
headings: list[str | None] = [None]
prev_heading = None
for i, element in enumerate(elements):
if isinstance(element, marko.block.Heading):
Expand All @@ -599,7 +606,7 @@ def _add_section_numbers(elements: Sequence[marko.element.Element]) -> None:
heading_level += 1
else:
headings.append(text_of(element))
heading_trace = " -> ".join(headings)
heading_trace = " -> ".join([str(heading) for heading in headings])
raise ValueError(
f"Encountered a level {element.level} heading ({text_of(element)}) at element {i} following a level {heading_level} heading ({prev_heading}); expected heading levels to increase by 1 level at a time. Trace: {heading_trace}"
)
Expand All @@ -612,4 +619,4 @@ def _add_section_numbers(elements: Sequence[marko.element.Element]) -> None:
else:
element.children = [
marko.block.inline.RawText(section_number)
] + element.children
] + element.children # pyright: ignore[reportOperatorIssue]
5 changes: 3 additions & 2 deletions monitoring/uss_qualifier/reports/report.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,12 +357,13 @@ def queries(self) -> list[fetch.Query]:
for case in self.cases:
for step in case.steps:
if step.has_field_with_value("queries"):
assert step.queries
queries.extend(step.queries)

if self.has_field_with_value("cleanup") and self.cleanup.has_field_with_value(
if self.has_field_with_value("cleanup") and self.cleanup.has_field_with_value( # pyright: ignore[reportOptionalMemberAccess]
"queries"
):
queries.extend(self.cleanup.queries)
queries.extend(self.cleanup.queries) # pyright: ignore[reportOptionalMemberAccess,reportArgumentType]

return queries

Expand Down
3 changes: 2 additions & 1 deletion monitoring/uss_qualifier/reports/sequence_view/events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def _step_events(
scenario_participants: dict[ParticipantID, TestedParticipant],
all_events: list[Event],
after: datetime | None,
) -> tuple[TestedStep, datetime]:
) -> tuple[TestedStep, datetime | None]:
events = []

# Create events for this step's passed checks
Expand Down Expand Up @@ -117,6 +117,7 @@ def _step_events(
for e in all_events:
if (
e.type == EventType.Query
and e.query is not None
and e.query.request.initiated_at == query_timestamp
):
query_events.append(e)
Expand Down
20 changes: 17 additions & 3 deletions monitoring/uss_qualifier/reports/sequence_view/generate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@

def _skipped_action_of(report: SkippedActionReport) -> ActionNode:
if report.declaration.get_action_type() == ActionType.TestSuite:
assert report.declaration.test_suite
if (
"suite_type" in report.declaration.test_suite
and report.declaration.test_suite.suite_type
Expand All @@ -76,6 +77,7 @@ def _skipped_action_of(report: SkippedActionReport) -> ActionNode:
)
name = "All actions in test suite"
elif report.declaration.get_action_type() == ActionType.TestScenario:
assert report.declaration.test_scenario
docs = get_documentation_by_name(report.declaration.test_scenario.scenario_type)
return ActionNode(
name=docs.name,
Expand All @@ -84,6 +86,7 @@ def _skipped_action_of(report: SkippedActionReport) -> ActionNode:
skipped_action=SkippedAction(reason=report.reason),
)
elif report.declaration.get_action_type() == ActionType.ActionGenerator:
assert report.declaration.action_generator
generator_type = action_generator_type_from_name(
report.declaration.action_generator.generator_type
)
Expand Down Expand Up @@ -123,20 +126,23 @@ def compute_action_node(report: TestSuiteActionReport, indexer: Indexer) -> Acti
is_action_generator,
) = report.get_applicable_report()
if is_test_scenario:
assert report.test_scenario
return ActionNode(
name=report.test_scenario.name,
node_type=ActionNodeType.Scenario,
children=[],
scenario=compute_tested_scenario(report.test_scenario, indexer),
)
elif is_test_suite:
assert report.test_suite
children = [compute_action_node(a, indexer) for a in report.test_suite.actions]
return ActionNode(
name=report.test_suite.name,
node_type=ActionNodeType.Suite,
children=children,
)
elif is_action_generator:
assert report.action_generator
generator_type = action_generator_type_from_name(
report.action_generator.generator_type
)
Expand All @@ -148,6 +154,7 @@ def compute_action_node(report: TestSuiteActionReport, indexer: Indexer) -> Acti
],
)
else:
assert report.skipped_action
return _skipped_action_of(report.skipped_action)


Expand Down Expand Up @@ -177,9 +184,13 @@ def _align_overview_rows(rows: list[OverviewRow]) -> None:
row.filled = True
to_fill -= 1
elif len(row.suite_cells) < max_suite_cols:
if row.suite_cells[-1].first_row and all(
c.node_type == ActionNodeType.Scenario
for c in row.suite_cells[-1].node.children
if (
row.suite_cells[-1].first_row
and row.suite_cells[-1].node is not None
and all(
c.node_type == ActionNodeType.Scenario
for c in row.suite_cells[-1].node.children
)
):
row.suite_cells[-1].colspan += max_suite_cols - len(row.suite_cells)
row.filled = True
Expand Down Expand Up @@ -212,6 +223,7 @@ def _align_overview_rows(rows: list[OverviewRow]) -> None:

def _enumerate_all_participants(node: ActionNode) -> list[ParticipantID]:
if node.node_type == ActionNodeType.Scenario:
assert node.scenario
return list(node.scenario.participants)
else:
result = set()
Expand All @@ -225,6 +237,7 @@ def _generate_scenario_pages(
node: ActionNode, config: SequenceViewConfiguration, output_path: str
) -> None:
if node.node_type == ActionNodeType.Scenario:
assert node.scenario
all_participants = list(node.scenario.participants)
all_participants.sort()
if UNATTRIBUTED_PARTICIPANT in all_participants:
Expand Down Expand Up @@ -308,6 +321,7 @@ def generate_sequence_view(
) -> None:
node = compute_action_node(report.report, Indexer())

assert report.configuration.v1 and report.configuration.v1.test_run
resources_config = make_resources_config(report.configuration.v1.test_run)

os.makedirs(output_path, exist_ok=True)
Expand Down
Loading
Loading