Test improvements

[Lotram]

Several improvements on tests:

• Better test isolation
  • All non-service tests now rely on fake service, config and issue objects
  • tests go through config validation only when this is actually needed, they instantiate config object directly otherwise
• TestTemplates inherits from ConfigTest instead of ServiceTest, as it does not need anything from `ServiceTest
• Move all service-related tests to dedicated services/ folder, to mirror source code
• remove the add_response static method, it became useless because of parameter deprecation. This allowed for simpler inheritance

This changes will also make #1134 and #1160 easier I think.

I tried to write several independent commits, I can create multiple PRs if that's easier.

[ryneeverett]

This does seem to improve the outcome as shown in the tests, but can you explain it? I take it that the problem had something to do with the early return without the descriminator Field? But what is the fundamental difference between _MissingServiceDiscriminator and ServiceConfig?

[Lotram]

It's related to _format_service_error error handling. When going through the ServiceConfig block,

1. We have an error where loc = (0, "service"), type = "missing" whereas the block we want in _format_service_error is if len(loc) == 1
2. Even if we change the condition to handle this, we would also get a loc = (0, "username") type = "extra_forbidden" error, because ServiceConfig does not know the service-specific fields. When using a discriminator field, if the discriminator raises an error, pydantic stop there and does not go through other errors.

This can also be modified in the error handling function, though I thought it was simpler to simply have an empty discriminator class

[ryneeverett]

I think I basically follow that explanation, but how does _MissingServiceDiscriminator avoid those issues?

[Lotram]

We need to have discriminated union to be as close as possible to the general case (when we have valid configs), for the reasons above.

so I guess your question is: why don't we fallback on service_config_classes = ServiceConfig instead of service_config_classes = _MissingServiceDiscriminator, when there is no valid service config ?

Simply because a discriminator requires the field (service here), to be a Literal, not a str

To validate models based on that information, you can set a common field on each model of the union (pet_type in the example below), typed as accepting one or multiple literal values.

The doc says:

To validate models based on that information, you can set a common field on each model of the union typed as accepting one or multiple literal values.

[ryneeverett]

Thanks, I get it now! I opened Lotram#2 as a proposed alternative which i find more readable and possibly more correct since service can't actually be an arbitrary str.

[Lotram]

I can see a problem with your implementation: Literal["__bugwarrior_service_placeholder__"] (type from the base class) is not compatible with Literal["github"], so there is a theoretical typing problem.

I agree service can't be an arbitrary string, it should be the Union of all declared service types, but since this registration is dynamic (and we don't want to register unused services), we have a problem. So IMO, str is "truer" than Literal["__bugwarrior_service_placeholder__"]

Here is a minimal example to reproduce:

from typing import Literal

from pydantic import BaseModel


class _String(BaseModel):
    service: str


class _Literal(BaseModel):
    service: Literal["_error_"]


class Config(_Literal):
    pass


class GitHubConfig(Config):
    service: Literal["github"]


class GitlabConfig(Config):
    service: Literal["gitlab"]

• ty is ok
• both mypy and pyright complain about the type (mypy error is: Incompatible types in assignment (expression has type "Literal['github']", base class "Config" defined the type as "Literal['_error_']"))

Note that pyright would still complain with the regular str instead of Literal, because "Variable is mutable so its type is invariant" (and it still complains when using frozen=True and `ClassVar), pyright complains a lot

So, as a conclusion, I'm ok with your implementation if you prefer it, for simplicity sake, but I think it's just a tiny bit more wrong

[ryneeverett]

No, let's stick with the more correct implementation. My only ask would be, please add at least a sentence of explanation to the _MissingServiceDiscriminator docstring about how it's essential purpose is to make the service field a Literal so it will work as a discriminator. I think using the ServiceConfig base class as a fallback here is the intuitive implementation -- after all it's what you did initially -- so this is a natural place for anyone to wonder why this class is necessary.