zulip / python-zulip-api

Python library for the Zulip API.
https://zulip.com/api/
Apache License 2.0
350 stars 352 forks source link

CI: Add test suites from zulip server. #711

Closed PIG208 closed 2 years ago

PIG208 commented 2 years ago

WIP. Testing the test setup on Github.

timabbott commented 2 years ago

@PIG208 this is awesome! I see that we're failing CI with the 18.04/3.2 test case.

The problem is kinda subtle: the API documentation that comes with 3.2 expects an old version of the python-zulip-api library's interface. I guess the bug is that we made a client-side API change that is not backwards-compatible, which was an intentional interface change, but does mean the 3.2 documentation isn't correct anymore.

If there was a magical way to do this, I guess what we want to promise to users and thus want to test is that the modern master API documentation, when run against the module python-zulip-api and an old server version will work correctly. I doubt it's particularly feasible to do that, since the test-api suite requires a bunch of code from the current server version for test database setup (etc.).

I think the next best thing we can do is try to hack around it -- e.g. figuring out a way to have a bit of configuration that lives in python-zulip-api that provides some way to do a compatibility hook for the tests. E.g. we have some mechanism through which if a certain environment variable is set, we rename a LegacyInterfaceClient class to Client, and that class overrides a few methods with thin wrappers to translate the interface for interface migrations. E.g. for the update_members_by_id function, it'd be something like this:

class LegacyInterfaceClient(Client)
    def update_user_group_members(self, group_data: Dict[str, Any]) -> Dict[str, Any]:
        modern_group_data = group_data.copy()
        group_id = group_data['group_id']
        del modern_group_data['group_id']
        return super().update_user_group_members(group_id, modern_group_data)

if USE_LEGACY_INTERFACE:
     Client = LegacyInterfaceClient  # type: ignore
PIG208 commented 2 years ago

Originally, I was thinking of making the exposed function signature to be something like def update_user_group_members(args, kwargs) and let the decorator handle the actual signature. This would make it easier to let us make the api compatible with arbritrary signatures, bypassing the below restriction:

One possible limitation of this approach to be noted: the decorated function should have a function signature compatible with any other alternatives.

However, type-checking (for dynamic function signature in terms of FEATURE_LEVEL) will be impossible with this approach. Then I guess hacking around will be the optimal solution here. But probably having an option to let the compatible decorator forcibly turn the decorated modern api to a legacy interface (which does not type-check, but works during runtime) would be a nicer hack here, since we can reuse some functionality defined in #710.

timabbott commented 2 years ago

For type-checking, one can use @override to define a couple compatible approaches.

That said, I think I'm increasingly convinced we want the library to just have the modern interface, and to the extent that we need to do hacks to make CI work, those should be contained in the CI, via the LegacyInterfaceClient approach I suggested above.

PIG208 commented 2 years ago

This and #710 are ready for review.

timabbott commented 2 years ago

This looks great @PIG208! I posted a few small comments, but this looks like something we'll be very happy to merge.

Does it make sense to skip the "api: Make update_user_by_id compatible. " commit for this PR (moving it to a separate one) so that we can merge this PR fully? And then we can just open a new PR with just that commit once this is merged.

PIG208 commented 2 years ago

Yes, I rebase this PR onto the feature level branch just to test it out.

On Mon, Oct 4, 2021 at 5:52 PM Tim Abbott @.***> wrote:

This looks great @PIG208 https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_PIG208&d=DwMCaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=b_yQYQzY71v5dLCdkk1wmw&m=Hbb8HIuFJgejQczESToR_U6kwZEAhC5glqFjO0pdXEw&s=sTd2fvRm5dKPNShTvCPZkNRPPlwcrl9gu_zmlSvqAVo&e=! I posted a few small comments, but this looks like something we'll be very happy to merge.

Does it make sense to skip the "api: Make update_user_by_id compatible. " commit for this PR (moving it to a separate one) so that we can merge this PR fully? And then we can just open a new PR with just that commit once this is merged.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_zulip_python-2Dzulip-2Dapi_pull_711-23issuecomment-2D933884187&d=DwMCaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=b_yQYQzY71v5dLCdkk1wmw&m=Hbb8HIuFJgejQczESToR_U6kwZEAhC5glqFjO0pdXEw&s=xU12xNPbSqUErZKKQfJ8Z_x4dapmujSs0Ngd4u17QAg&e=, or unsubscribe https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AJQG4X2I22ZBRE5VKTQHY2LUFIO2RANCNFSM5BVRQRMQ&d=DwMCaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=b_yQYQzY71v5dLCdkk1wmw&m=Hbb8HIuFJgejQczESToR_U6kwZEAhC5glqFjO0pdXEw&s=OuoISmfpr2Rswo1xEkBC55k0mca6jRDekniD6pILkZI&e= .

PIG208 commented 2 years ago

OK. I will come back to this on weekend.

timabbott commented 2 years ago

This looks great; I did some updates to the documentation and set the legacy_client_interface parameter to 4.0 for the last two tests.

timabbott commented 2 years ago

Merged, thanks @PIG208!