feat: guess subdomains based on conventions (DEV-1979)

[jnussbaum]

No description provided.

[linear]

DEV-1979 DSP-TOOLS: guess subdomains based on conventions

Problem

https://docs.dasch.swiss/2023.03.02/DSP-TOOLS/cli-commands/#before-starting-have-in-mind-the-subdomains-of-a-dsp-server explains how to address the subdomains of a DSP server.

But if a user makes the mistake to address a server with admin.dasch.swiss instead of api.dasch.swiss, he doesn't get a useful error message. Even worse: If he forgets to specify the SIPI server for an xmlupload, DSP-TOOLS tries to send the multimedia files to a localhost SIPI, which leads to cryptic error messages.

Solution

Don't care about subdomains at all, and just rely on the convention that admin=DSP-APP, api=DSP-API, iiif=SIPI. We could then get rid of this complicated paragraph that makes user's lives difficult: https://docs.dasch.swiss/2023.03.02/DSP-TOOLS/cli-commands/#before-starting-have-in-mind-the-subdomains-of-a-dsp-server.
I could then entirely get rid of the -S https://iiif.dasch.swiss flag.

[linear]

DEV-1979 DSP-TOOLS: guess subdomains based on conventions

Problem

https://docs.dasch.swiss/2023.03.02/DSP-TOOLS/cli-commands/#before-starting-have-in-mind-the-subdomains-of-a-dsp-server explains how to address the subdomains of a DSP server.

But if a user makes the mistake to address a server with admin.dasch.swiss instead of api.dasch.swiss, he doesn't get a useful error message. Even worse: If he forgets to specify the SIPI server for an xmlupload, DSP-TOOLS tries to send the multimedia files to a localhost SIPI, which leads to cryptic error messages.

Solution

Don't care about subdomains at all, and just rely on the convention that admin=DSP-APP, api=DSP-API, iiif=SIPI. We could then get rid of this complicated paragraph that makes user's lives difficult: https://docs.dasch.swiss/2023.03.02/DSP-TOOLS/cli-commands/#before-starting-have-in-mind-the-subdomains-of-a-dsp-server.
I could then entirely get rid of the -S https://iiif.dasch.swiss flag.

[BalduinLandolt]

I added some comments, feel free to apply or ignore what you see fit

[BalduinLandolt]

I'm not a fan of this kind of test parameterisation: if you want to test arbitrary strings because you're concerned the behaviour might be flaky, then you should use a framework for property-based testing (like Hypothesis); otherwise you should identify the different possible code paths and test those systematically with one test per possible case.
If you expect a different behaviour for "082a-test-server.dasch.swiss" and "http://api.dev.dasch.swiss/" then this test either tests the regex you wrote (if you don't trust that) or the correctness of the python regex implementation. If the first is the case, then factor out your regexes and test them separately, if the latter is the case, then don't! 😉

And if you reduce the number of cases you want to test, then you don't need to parametrise your tests, and instead you can have a dedicated test method for each case you want to test, which gives you clearer and more expressive test results.

[jnussbaum]

I think there is another case: I'd like my unit tests to be an executable requirements specification. I think I have read that in David Farley's "Modern Software Engineering", but I'm not sure.

The idea that I have in mind: Some day, someone might come across and modify my code, so that by mistake, one of the subdomains isn't covered any more. So I want my unit tests to be a protection against future harm.

Does that make sense?

[BalduinLandolt]

Makes sense, but I argue that it's a trade-off: Of course better coverage gives confidence for future code changes. But at the same time, test cases come with a maintenance cost - if requirements change, not only production code but also test code has to be adopted. That's why the granularity and amount of tests are a balance to get right.
(That's an issue we're struggling with, both in the API and the JS-LIB: We have huge amounts of "reference test data" and normally it takes longer to update "fix" all the tests afterwards than to implement an actual feature. I don't think this case is comparable - but I'm a bit hyper-sensitive to that now... 😉 )

If you feel that testing all this helps, then I'm 100% fine with it, but it may not be the kind of testing I currently like to do.

[jnussbaum]

Okay - I see your point, and will keep it in mind. thanks for the input, it's always interesting to exchange thoughts with you, and I can learn a lot from your comments :-)

[BalduinLandolt]

see my comment above on the information flow: if _derive_sipi_url() is changed to a str -> tuple[str, str] function, then this test can be simplified a lot:

        api, sipi = _derive_sipi_url("localhost:3333") # or whatever you want to test
        self.assertEqual(api, "http://0.0.0.0:3333")
        self.assertEqual(sipi, "http://0.0.0.0:1024")

and as such it's also not too much to have separate test methods:

    def test_derive_sipi_localhost(self) -> None:
        api, sipi = _derive_sipi_url("localhost:3333")
        self.assertEqual(api, "http://0.0.0.0:3333")
        self.assertEqual(sipi, "http://0.0.0.0:1024")

    def test_derive_sipi_localhost_zeros(self) -> None:
        api, sipi = _derive_sipi_url("0.0.0.0:3333")
        self.assertEqual(api, "http://0.0.0.0:3333")
        self.assertEqual(sipi, "http://0.0.0.0:1024")

etc.