First pytest migration

[Lotram]

First commit

Fix a test in test_service.py : since this commit the test check each abstract method is defined only once, which does not really make sense. I changed it back to check the number of calls, while adding an allowlist for some abstract methods we do want to call from concrete methods.

An alternative could be to simply remove this test, since we already have an exception

Second commit

Migrate two test classes to pytest (remove TestCase inheritance + plain asserts), to ensure we agree on the migration.

Note that the changes in test_db are not that straightforward because merge_left mutates its first arg and replace_left mutates both, which is error-prone IMO. Returning a whole new dict (without modifying inputs) would be safer

[ryneeverett]

Regarding the first commit -- oops! Thanks for catching this.

It isn't clear to me that an exception is justified for get_keyring_service though. Perhaps this would be a good time to eliminate the method in favor of a class property string on which Service.get_secret calls .format(config)?

[Lotram]

I'm ok with removing get_keyring_service, but it wouldn't work right away with KanboardService and PhabricatorService:

    def get_keyring_service(config: KanboardConfig) -> str:
        parsed = urlparse(config.url)
        return f"kanboard://{config.username}@{parsed.netloc}"

One solution would be to use a computed_field for the host, in both KanboardConfigandPhabricatorConfig`

Another solution would be to make get_keyring_service a method of the ServiceConfig subclasses, instead of the Service ones ? IMO it makes sense to attach this to the config. Users could want to customize this in their config, if we ever want to support this.

[ryneeverett]

The computed_field solution seems good. Making this a string rather than a method is the main thing I'd like to see.

I'd be ok with that string moving to ServiceConfig, but I wouldn't want to see the get_keyring_service method moved to ServiceConfig. As you probably realize, it shouldn't be configurable at this point since there is no apparent demand for that.