Test installer's raid1 feature

[ydirson]

This builds on top of #226

[ydirson]

Last push rebases, and fixes the issues in AnswerFile dict typechecking

[glehmann]

can't we check that self.defn['CONTENTS'] is a list or a Sequence here?

[ydirson]

The goal of this is to be as close as possible for checking that self.defn is indeed of a subclass of its formal type hint. I had a quick try at actually writing a type hint for this but got into problems (which does not mean it is not feasible, I chose concentrated on other things at that time).

self.defn is typed SimpleAnswerfileDict, so self.defn['CONTENTS'] is NotRequired[Union[str, "SimpleAnswerfileDict", Sequence["SimpleAnswerfileDict"]]].

The code assumes a "toplevel" is rather typed Required[Union["SimpleAnswerfileDict", Sequence["SimpleAnswerfileDict"]]], so:

• if we stick we assert I missed an assert "CONTENTS" in self.defn first
• the problem with asserting the actual types is that isinstance cannot check using those type hints, which is why I chose to check self.defn is just not in the complementary set

But yes, this is a bit hackish and should be better explained in a comment, or simply typed better.

[glehmann]

pydantic could help here:

class Foo(TypedDict):
    bar: int
    baz: str
    plop: NotRequired[None | str]
foo_validator = TypeAdapter(Foo)
# raise an exception when the arg doesn't match what's declared in Foo
foo_validator.validate_python(dict(bar=1, baz="blah", plop=None))

[ydirson]

It would be more pythonic to keep as much as possible at the type-hint level :)

[glehmann]

That's using the type hints, just replacing the assert with the pydantic validator:

from typing import TypedDict
from typing_extensions import NotRequired
from pydantic import TypeAdapter
import os

class Foo(TypedDict):
    bar: int
    baz: str
    plop: NotRequired[None | str]
foo_validator = TypeAdapter(Foo)

v = None
if os.path.exists("/tmp"):
    v = dict(bar=1, baz="blah", plop=None)
# at that point v is of type dict[str, int | str | None]

v = foo_validator.validate_python(v)
# here v is of type Foo

[ydirson]

I mean, that makes the runtime check more expensive (the reason why type hints are ignored by the interpreter)

[glehmann]

Sure, but it's still very fast compared to almost everything we're doing in the tests

In [2]: %timeit foo_validator.validate_python(v)
209 ns ± 1.35 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

[glehmann]

IMO it would have been simpler to keep the old implementation (before the previous commit that prepared this one) and just cast the result to the expected type.
Do we have some benefit to do it that way, with a cast and a type: ignore?

[glehmann]

I think 'pydantic' can validate a TypedDict. We could think of using it, independently of the solution, that both require a cast.

[ydirson]

IMO it would have been simpler to keep the old implementation (before the previous commit that prepared this one)

Not sure which commit you mean, its name would help 🙂
oh "do the _normalize_structure copy more manually"

[ydirson]

Do we have some benefit to do it that way, with a cast and a type: ignore?

The problem is, it cannot be just a cast. The original implementation gradually mutates a SimpleAnswerfileDict into a AnswerfileDict, and the typechecker is not smart enough.

on:

            defn['CONTENTS'] = [AnswerFile._normalize_structure(item)
                                for item in defn['CONTENTS']]

pyright:

  /home/user/src/xcpng/tests/lib/installer.py:47:13 - error: Could not assign item in TypedDict
    Type "list[AnswerfileDict]" is not assignable to type "str | SimpleAnswerfileDict | Sequence[SimpleAnswerfileDict]"
...
      "list[AnswerfileDict]" is not assignable to "Sequence[SimpleAnswerfileDict]"
        Type parameter "_T_co@Sequence" is covariant, but "AnswerfileDict" is not a subtype of "SimpleAnswerfileDict"
          "CONTENTS" is not required in "SimpleAnswerfileDict"
          "CONTENTS" is an incompatible type
            Type "str | list[AnswerfileDict]" is not assignable to type "str | SimpleAnswerfileDict | Sequence[SimpleAnswerfileDict]"

I tried quite a few things and that quickly becomes a nightmare of casts which defeats the type-checking. Right now I don't see a better way to write _normalize_structure, and that type: ignore is only for mypy, pyright does not seem to have any issue there (which might be another issue, I would expect it to require us to duplicate the known-attributes for AnswerfileDict).

Anyway, if we move those attributes into a separate dict as discussed in a thread below, the need for that override will vanish.

[glehmann]

This would raise an AssertionError if defn is a ValueError. Shouldn't we raise the ValueError directly?

[ydirson]

Well, the whole idea of ValueError here is that we'd like to get it thrown in the first place, so we might not need it in the type any more if we switch from ternary operator to dict lookups.

Else yes, that makes sense.

[glehmann]

system_disk_config could be validated here.

Maybe use an enum to represent the possible values?

Also disk could have a more explicit name like single_disk.

[ydirson]

system_disk_config could be validated here

do you mean type-hinted?

Also disk could have a more explicit name like single_disk.

I am not very happy with disk but those land in the test name, so I'd prefer a short alternative. For context, after piling IPv6 parameter on top of this, the test are like:

tests/install/test.py::TestNested::test_boot_inst[uefi-83nightly-host1-disk-iso-nosr-ipv6static]

And we are not anywhere near what they ought to be ☹️

[glehmann]

system_disk_config could be validated here

do you mean type-hinted?

I mean validated at run time to make sure that the value is a valid one.

An enum could help also with the static analysis. I think that mypy/pyright are able to check when we are not checking all the possible variants

[ydirson]

That rather sounds like something the type checkers should be aware of: pytest.mark.parametrize indeed declares an enum, but declaring it ourselves for every parameter 1. seems wrong 2. is likely to make the code less readable

[glehmann]

I'm not a fan of nesting conditional expressions. I think I would prefer something like:

dict(raid1='md127', disk=system_disks_names[0])[system_disk_config]

[ydirson]

I used those in a few places (I do like getting in a single short line what would otherwise take 4), but in this particular place we'll want to be consistent and not mix the 2 idioms, and I'm not sure how readable that would get with the larger expressions.
OTOH, getting real ValueError raised instead of having to add them in the type hints has a lot of appeal... this needs to be looked at.

[glehmann]

why didn't you raise the ValueError in the first place if that's an acceptable option?

I think that

def raise_value_error(v) -> SimpleAnswerfileDict:
    raise ValueError(v)

{"TAG": "raid", "device": "md127",
  "CONTENTS": [
     {"TAG": "disk", "CONTENTS": diskname} for diskname in system_disks_names
 ],
} if system_disk_config == "raid1"
else None if system_disk_config == "disk"
else raise_value_error(f"system_disk_config {system_disk_config!r}")

should have worked (not tasted). Am I missing something?

[glehmann]

dict(
    raid1={
        "TAG": "raid",
        "device": "md127",
        "CONTENTS": [
            {"TAG": "disk", "CONTENTS": diskname} for diskname in system_disks_names
        ],
    },
    disk=None,
)[system_disk_config]

still seems much easier to read to me :)

[ydirson]

Unfortunately it seems we hit a mypy limitation, with

        .top_append(
            {"iso": {"TAG": "source", "type": "local"},
             "net": {"TAG": "source", "type": "url",
                     "CONTENTS": ISO_IMAGES[iso_version]['net-url']},
             }[package_source],
...

gets it complaining:

tests/install/test.py:84: error: Argument 1 to "top_append" of "AnswerFile" has incompatible type "dict[str, str]"; expected "SimpleAnswerfileDict | ValueError | None"  [arg-type]

... where the values of that dict are obviously valid SimpleAnswerfileDict's

and the following does not look near as neat:

            cast(SimpleAnswerfileDict, {"iso": {"TAG": "source", "type": "local"},
                                        "net": {"TAG": "source", "type": "url",
                                                "CONTENTS": ISO_IMAGES[iso_version]['net-url']},
                                        }[package_source]),