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

update to pyicloud-1.0.0 to support 2fa verification #528

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

qzchenwl
Copy link

@AndreyNikiforov
Copy link
Collaborator

@qzchenwl looks like you are removing dependency on pyicloud fork - a long standing request #179 . That is awesome and thank you very much for the effort.

How can we validate that such massive change does not break anything?

As a small step, may be we can make existing tests working? I recently fixed pipeline and all tests on master are green. Can you rebase/pull forward from the master to take advantage of the recent fixes in quality checks, please?

@qzchenwl
Copy link
Author

qzchenwl commented Jan 19, 2023

@qzchenwl looks like you are removing dependency on pyicloud fork - a long standing request #179 . That is awesome and thank you very much for the effort.

How can we validate that such massive change does not break anything?

As a small step, may be we can make existing tests working? I recently fixed pipeline and all tests on master are green. Can you rebase/pull forward from the master to take advantage of the recent fixes in quality checks, please?

I tried to make existing tests working #536. I've updated the Python code part of the unit test, but I failed to update the http request mocking because I am not familiar with pytest and vcr.

The reason we need to update http request mocking is that pyicloud-1.0.0 use https://idmsa.apple.com/appleauth/auth/signin to authenticate while current test case only mocks https://setup.icloud.com/setup/ws/1/login. (pytest for test_authentication.AuthenticationTestCase.test_failed_auth)

What's the best practice to mock new http request (maybe record and save real requests) without using a real Apple ID?

@qzchenwl
Copy link
Author

@AndreyNikiforov I don't think http mocking style unit test is wrong way to test, because it is not the responsibility
of icloud_photos_downloader to abstracting the implementation detail (rest api), it is the responsibility of pyicloud. The unit tests should mock the public api of pyicloud.

@menkej
Copy link
Collaborator

menkej commented Jan 19, 2023

What's the best practice to mock new http request (maybe record and save real requests) without using a real Apple ID?

This is the way I'd go. Just record a new request using a valid Apple ID and then remove the Credentials from the recording before checking it in.

@AndreyNikiforov
Copy link
Collaborator

@AndreyNikiforov I don't think http mocking style unit test is wrong way to test, because it is not the responsibility of icloud_photos_downloader to abstracting the implementation detail (rest api), it is the responsibility of pyicloud. The unit tests should mock the public api of pyicloud.

@qzchenwl I agree that having unit test for our logic and give responsibility for testing http parsing to pyicloud is a good approach.

We can still benefit for e2e tests to confirm that our product works as intended. I see existing tests with http request mocking kinda e2e (but without real connection). Even though they are not making real connection they are still a good protection against regressions and we better keep them relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment