Dm/interface registration test

[DarkaMaul]

This PR opens some discussions on wherever we want to try to detect if interfaces are registered in the tests.

On the pro side, it would have caught a problem like pypi#16302 .

On the other side:

• I think the current implementation is brittle
• @woodruffw raised a concern about some features being gated (e.g. Beta) and purposefully not registered

The current test is failing because IOriginCache is not registered.
Indeed, the registration is under a flag :

def includeme(config):
    if "origin_cache.backend" in config.registry.settings:
        cache_class = config.maybe_dotted(
            config.registry.settings["origin_cache.backend"]
        )
        config.register_service_factory(cache_class.create_service, IOriginCache)
        config.add_view_deriver(html_cache_deriver)

    config.add_directive("register_origin_cache_keys", register_origin_cache_keys)

source

This is not a problem because most of the calls to the find_service method are called in a try/catch block:

    try:
        cacher_factory = config.find_service_factory(IOriginCache)
    except LookupError:
        return

source

However, this is not always the case :

• purge_key

[DarkaMaul]

/cc @facutuesca @woodruffw

[woodruffw]

Thanks for the detailed writeup @DarkaMaul!

The docs changes look good to me, and make a lot of sense to send as an independent PR.

In terms of detecting service (non-)registration: I'm not a huge fan of doing this via inspect (I agree that's going to be too brittle), but I am coming around to the idea that we should test for service registration somehow.

Instead of trying to do this through introspection, we could perhaps do this through a list of known interfaces + the regular config loading flow.

For example:

def test_all_interfaces_registered():
    # these lists will need to be updated by hand whenever a new interface is added,
    # similar to how other config tests get updated whenever Warehouse's config changes
    known_interfaces = [IOIDCPublisherService, ...]
    ignored_interfaces = [IOriginCache, ...]

	# see body of test_configure for the rest of the needed state here
    configurator_obj = pretend.stub(register_service_factory=pretend.call_recorder(...))
    config.configure(None)

    for call in configurator_obj.register_service_factory.calls:
        # check all registered interfaces against known_interfaces/ignored_interfaces
		...

I think that will be much more reliable, at the cost of being slightly more manual (but the config unit tests are already pretty manual, on purpose). Thoughts?

[DarkaMaul]

Thanks for the detailed writeup @DarkaMaul!

The docs changes look good to me, and make a lot of sense to send as an independent PR.

The PR is open here: #2827
This PR/branch is just based on it, but I can also rebase it on main.

In terms of detecting service (non-)registration: I'm not a huge fan of doing this via inspect (I agree that's going to be too brittle), but I am coming around to the idea that we should test for service registration somehow.

Instead of trying to do this through introspection, we could perhaps do this through a list of known interfaces + the regular config loading flow.

For example:

def test_all_interfaces_registered():
    # these lists will need to be updated by hand whenever a new interface is added,
    # similar to how other config tests get updated whenever Warehouse's config changes
    known_interfaces = [IOIDCPublisherService, ...]
    ignored_interfaces = [IOriginCache, ...]

	# see body of test_configure for the rest of the needed state here
    configurator_obj = pretend.stub(register_service_factory=pretend.call_recorder(...))
    config.configure(None)

    for call in configurator_obj.register_service_factory.calls:
        # check all registered interfaces against known_interfaces/ignored_interfaces
		...

I think that will be much more reliable, at the cost of being slightly more manual (but the config unit tests are already pretty manual, on purpose). Thoughts?

IMO, having the interface list configured manually is less helpful here.
My initial idea was to catch the problem when someone (like me) creates an interface but forgets to register it.
Maybe a test is not the best place to put this, and a lint-style check would make more sense?

[woodruffw]

IMO, having the interface list configured manually is less helpful here.
My initial idea was to catch the problem when someone (like me) creates an interface but forgets to register it.
Maybe a test is not the best place to put this, and a lint-style check would make more sense?

Ah yeah, I didn't think this through -- the list won't catch the non-registration case.

A lint seems possible -- I think it'd be straightforward-ish to write a pylint check that looks for disjunctions between calls to find_service(iface) and calls to register_service_factory. We'd probably want to make this flow-insensitive, i.e. so that registrations behind feature flags don't result in a false positive.

OTOH, that'll further constrain Warehouse to pylint (I'd love to switch it over to ruff at some point) and it's a little removed from our current scope. So IMO let's defer on that for now, and stick with just docs changes.

[woodruffw]

The PR is open here: #2827
This PR/branch is just based on it, but I can also rebase it on main.

I think you meant #3046, right?