[uss_qualifier/reports] Typing fixes

[the-glu]

updated ./.basedpyright/baseline.json with 2462 errors (went down by 94)
0 errors, 0 warnings, 0 notes

Mostly assert & ignore, as typing don't understand hasattr, and *Type system (nor has_field_with_value, and narrowing is not smart enough yet to indicate we perform the test)

[BenjaminPelletier]

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 == ...

[the-glu]

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?

[BenjaminPelletier]

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")

[the-glu]

Should we go with an Union then`?

class CaseEpoch(ImplicitDict):
    case: TestedCase
    type: Literal[EpochType.Case] = EpochType.Case

    @property
    def rows(self) -> int:
        return self.case.rows


class EventsEpoch(ImplicitDict):
    events: list[Event]
    type: Literal[EpochType.Events] = EpochType.Events

    @property
    def rows(self) -> int:
        return len(self.events)


Epoch = CaseEpoch | EventsEpoch

Basedpyright is happy, no need for asset, and I do think that cleaner in term of implementation (no if tree per function, just a dedicated function per sub class type).

However this mean that we need to update the code to instantiate class of the correct type:

        epochs.append(Epoch(case=TestedCase(name="", url="#", steps=[tested_step])))

->

        epochs.append(CaseEpoch(case=TestedCase(name="", url="#", steps=[tested_step])))

or we do use a helper:

def init_epoch(
    case: TestedCase | None = None, events: list[Event] | None = None
) -> Epoch:

    if case:
        return CaseEpoch(case=case)
    elif events:
        return EventsEpoch(events=events)
    else:
        raise ValueError("Invalid Epoch did not specify case or events")

(Extenting the Union directly doesn't seems to be possible without heavy hacks)

What do you think?

[BenjaminPelletier]

I think that's probably a great solution when all the classes in question are dataclasses rather than ImplicitDicts. The primary use of ImplicitDict is to support serialization and deserialization, and the reason protocol buffers use the oneof pattern rather than a polymorphic field is that polymorphic deserialization is hard (whereas deserialization with oneof is easy because the polymorphic types are encoded in the alternate field names). Concretely, ImplicitDict can't deserialize polymorphic fields, and that isn't even possible in general (though identical Literal type fields acting as a discriminator make it possible here). Example:

from enum import Enum
import json
from typing import Literal

from implicitdict import ImplicitDict


class EpochType(str, Enum):
    Case = "Case"
    Events = "Events"


class CaseEpoch(ImplicitDict):
    case: str
    type: Literal[EpochType.Case] = EpochType.Case

    @property
    def rows(self) -> int:
        return self.case.rows


class EventsEpoch(ImplicitDict):
    events: list[str]
    type: Literal[EpochType.Events] = EpochType.Events

    @property
    def rows(self) -> int:
        return len(self.events)


class EpochContainer(ImplicitDict):
    epoch: CaseEpoch | EventsEpoch


container_src = EpochContainer(epoch=CaseEpoch(case="foo"))
container_dst = ImplicitDict.parse(json.loads(json.dumps(container_src)), EpochContainer)

We could perhaps modify ImplicitDict to look for a Literal field common to all types in the polymorphism in order to support polymorphic fields, but then I worry about outside-codebase compatibility. Currently, it should be pretty easy for a user to grab the JSON Schema for a particular type (e.g., TestRunReport), automatically build an object in their preferred language, and deserialize into that object. But, if we add polymorphic fields, only languages and importers that support polymorphism will be able to easily consume our data. When I checked a few years ago, not many OpenAPI code generators supported polymorphism even though JSON Schema does have oneOf; Gemini suggests this may still be the case:

While there is no official central database tracking the exact percentage, industry analysis and user reports suggest that approximately 15% to 30% of OpenAPI code generation tools provide robust, "out-of-the-box" support for polymorphic fields using oneOf or anyOf.

Support for polymorphism is widely considered the "Achilles' heel" of the OpenAPI ecosystem. Even within high-profile projects like OpenAPI Generator, support varies significantly between language-specific templates (e.g., the Java/Spring generator may support it while the Go or Python generators might not).

So maybe just inspecting the oneOf fields directly and not switching on the type property accessor is the least-bad approach for now?

[the-glu]

So maybe just inspecting the oneOf fields directly and not switching on the type property accessor is the least-bad approach for now?

Yes true, I forgot about parsing implications with that solution. Let's go with inspection for now.

[BenjaminPelletier]

I think these follow the oneof pattern so I think we want to inspect the fields directly rather than switch on the type property accessor (same comment for similar assertions below)

Suggested change

	if report.declaration.get_action_type() == ActionType.TestSuite:
	assert report.declaration.test_suite
	if "test_suite" in report and report.declaration.test_suite:

[BenjaminPelletier]

I think this is the oneof pattern again, so I think we should probably (same comment for similar assertions below):

Suggested change

	assert report.test_scenario
	if "test_scenario" in report and report.test_scenario:

(note: GitHub suggestion formatting is funky; this one line would replace lines 121-127)

[BenjaminPelletier]

oneof pattern again

[BenjaminPelletier]

Since we have to repeat ourselves here any way for the benefit of basedpyright, it seems like has_field_with_value isn't providing any value; let's just write the logic directly:

Suggested change

	if step.has_field_with_value("queries"):
	assert step.queries
	if "queries" in step and step.queries:

I think this will probably apply most places we currently use has_field_with_value.