Boavizta / boaviztapi

🛠 Giving access to BOAVIZTA reference data and methodologies trough a RESTful API
GNU Affero General Public License v3.0
66 stars 21 forks source link

Tests(cloud): reduce verbosity of cloud tests #272

Closed Shillaker closed 3 months ago

Shillaker commented 4 months ago

Changes

Why?

The cloud tests contain several blocks of repeated text and logic, which means adding new cloud tests requires a lot of copy-pasting. This should make the tests easier to read, and make it easier to add new ones.

Details

I have only refactored the existing tests, so although there is a large diff, it is exactly the same logic, just rearranged. I can recommend looking at the diff side-by-side, and checking the final version of the test_cloud.py file in the branch here.

da-ekchajzer commented 4 months ago

Thank you for this PR. It is much appreciated, I will extend it to all the tests when I have time.

Could you add a commit where you ran "poetry lock --no-update" to update the poetry lock. It seems that this file is out of date.

Shillaker commented 4 months ago

Ok great, I've rebased onto main, so the pipeline should work now.

I'm happy to do the work to extend to the other tests. I will do it in chunks in other PRs to avoid creating a single massive diff.

da-ekchajzer commented 4 months ago

We are still maintaining the compatibility with python 3.8. Since the CI of test uses python 3.8 you have to use List from typing instead of list.

The error :

tests/api/util.py:17: in ImpactOutput
    warnings: Optional[list[str]] = None
E   TypeError: 'type' object is not subscriptable

More on that : https://stackoverflow.com/questions/63460126/typeerror-type-object-is-not-subscriptable-in-a-function-signature

Could you use a List from typing in tests/api/util.py:17 and test with a python 3.8 environnment ?

Sorry for that we have plan to upgrade see : https://github.com/Boavizta/boaviztapi/issues/251

I'm happy to do the work to extend to the other tests. I will do it in chunks in other PRs to avoid creating a single massive diff.

Any contribution on this would be highly appraciated. It the kind of things that improves code quality making test easyier to implement and manage.

Shillaker commented 4 months ago

Ah sorry @da-ekchajzer I had not realised we were still aiming for Python 3.8.

I see you are in the middle of a PR to update to 3.10. I understand we want to maintain compatibility with 3.8 for the main codebase, but is it still necessary for the tests? Let me know if I should continue with the 3.8 change after your PR.

da-ekchajzer commented 3 months ago

Thank you for your attentiveness. Your PR started the discussion about maintaining python3.8. We have decided to drop python 3.8 and maintain the API for python >= 3.9. I'll merge this PR once https://github.com/Boavizta/boaviztapi/pull/278 is merged.

Let me know if I should continue with the 3.8 change after your PR.

No, you don't have to change the PR to make it compatible with python 3.8