Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Mar 7, 2024

Conversation

Shillaker
Copy link
Contributor

@Shillaker Shillaker commented Feb 20, 2024

Changes

  • Add some utility classes that template requests and expected responses
  • Refactor existing cloud tests using these classes

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.

@Shillaker Shillaker changed the title Tests(cloud): reduce duplication in cloud tests Tests(cloud): reduce verbosity of cloud tests Feb 20, 2024
@da-ekchajzer
Copy link
Collaborator

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 Shillaker force-pushed the cloud-test-dedupe branch from 8955eef to 37be93e Compare March 2, 2024 11:54
@Shillaker
Copy link
Contributor Author

Shillaker commented Mar 2, 2024

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
Copy link
Collaborator

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 : #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
Copy link
Contributor Author

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
Copy link
Collaborator

da-ekchajzer commented Mar 6, 2024

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 #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

@da-ekchajzer da-ekchajzer merged commit a30f880 into Boavizta:main Mar 7, 2024
1 check failed
@Shillaker Shillaker deleted the cloud-test-dedupe branch March 13, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants