vorta Tests leave key in keyring

Adding remove password abstract method in abc.py and implementing in secretstorage.py. @real-yfprojects

Mar 09 '23 16:03 ganeshrevadi

I'll try macOS today.

Mar 12 '23 08:03 m3nu

Added for macOS:

In [1]: from vorta.keyring import darwin

In [2]: k = darwin.VortaDarwinKeyring()

In [3]: k.get_password('vorta', 'google.com')

In [4]: k.set_password('vorta', 'google.com', 'asdf')

In [5]: k.get_password('vorta', 'google.com')
Out[5]: 'asdf'

In [6]: k.remove_password('vorta', 'google.com')

In [7]: k.get_password('vorta', 'google.com')

In [8]:

Mar 12 '23 12:03 m3nu

That's bad you just removed the commits containing MacOS support.

Mar 12 '23 15:03 real-yfprojects

That's bad you just removed the commits containing MacOS support.

Hey I'm really sorry how can this be fixed now?

Mar 12 '23 16:03 ganeshrevadi

Rebased and pushed it again.

Please be careful with force-push and use it only when really needed.

Mar 12 '23 19:03 m3nu

And when you do use --force-with-lease.

Mar 12 '23 19:03 real-yfprojects

And when you do use --force-with-lease.

when you update a branch changed by other people

Mar 13 '23 03:03 ganeshrevadi

should remove_password next implemented in db.py?

Mar 13 '23 03:03 ganeshrevadi

For macOS-related stuff it's needed because some libraries aren't available on linux (pyobjc).

In this case it could be due to circular dependencies. Needs to be reviewed. If no issues, put it at the start of the file, where it belongs.

Mar 13 '23 15:03 m3nu

For macOS-related stuff it's needed because some libraries aren't available on linux (pyobjc).

In this case it could be due to circular dependencies. Needs to be reviewed. If no issues, put it at the start of the file, where it belongs.

I tried adding it above but its failing the pre-commit run!

Mar 15 '23 15:03 ganeshrevadi

I tried adding it above but its failing the pre-commit run!

As always, nobody can help you if you just say that it failed. You will have to provide more details. The error/complaint message is a minimum.

Mar 15 '23 16:03 real-yfprojects

I have imported the RepoPassword at beginning of the file and it works fine and sorry for the lack of info in above comment.

Mar 16 '23 14:03 ganeshrevadi

You can use the newly implemented method now to remove passwords saved in test_utils.py and test_repo.py.

Mar 16 '23 19:03 real-yfprojects

You can use the newly implemented method now to remove passwords saved in test_utils.py and test_repo.py.

Can you please explain about test_repo.py?

Mar 17 '23 17:03 ganeshrevadi

Can you please explain about test_repo.py?

Well, the main purpose of this PR was to avoid having hundreds of old passwords in our keychain from tests. Since our tests added a random repo and password, but never removed it. So you can now adjust the tests to remove the password when it's done. Best to see the pytest docs on where to best do this cleanup. The cleanup should also happen if the test fails.

Mar 17 '23 17:03 m3nu

I have tried thinking of implementing remove_password method in test_repo.py but as my per my observation there is no case where password is getting saved(May be I'm wrong about this idk). I'm out of a possible solution , can someone help me here?

Mar 23 '23 13:03 ganeshrevadi

https://github.com/borgbase/vorta/blob/master/tests/test_repo.py#L68 https://github.com/borgbase/vorta/blob/master/tests/test_repo.py#L84

Mar 23 '23 14:03 m3nu

@m3nu I've added the remove_password method in the test_repo.py.

Mar 23 '23 16:03 ganeshrevadi

Looks good. I just have these concerns:

The assert to test if the password is gone isn't really needed all the time. Just enough to test it one time where the keyring is tested. You wouldn't test everything all the time. One unit, one test (unit test). (or integration test if you test clicking buttons, etc)
What happens if the test fails in the middle? Then the password is never removed. For this reason pytest uses cleanup functions and fixtures. Would be better to use those. I'm not sure what's the best way in this case. Best to quickly consult the pytest docs.

Mar 23 '23 16:03 m3nu

@m3nu I've added the pytest-fixture , can you please verify?

Mar 24 '23 14:03 ganeshrevadi

Not quite.

The keyring instance you create in the fixture isn't used in the test and Vorta will always make its own.
You wouldn't need to remove the password inside tests, if the fixture was working. keyring.remove_password('vorta-repo', test_repo_url)
Your fixture runs at the module level. So all tests in one file would use the same password and remove it??

So what you need is this:

Fixture that runs again for each test
Fixture that returns some random repo URL and random password for testing, as we do in some places: test_repo_url = f'vorta-test-repo.{uuid.uuid4()}.com:repo' # Random repo URL to avoid macOS keychain and password = str(uuid.uuid4())
The repo URL and password are then used during testing
After each test function the password for the test repo is removed again.

This should be more clear and deduplicates some more code (creating a random repo URL and password)

One more thing: vorta-test-repo.{uuid.uuid4()}.com:repo would give a SFTP-style URL that's not supported in the future. Best if you change it to ssh:// format.

Mar 24 '23 14:03 m3nu

Also be sure to rebase your branch sometimes, since the master branch will keep changing when a PR is open for longer.

Mar 24 '23 14:03 m3nu

Not quite.

The keyring instance you create in the fixture isn't used in the test and Vorta will always make its own.

You wouldn't need to remove the password inside tests, if the fixture was working. keyring.remove_password('vorta-repo', test_repo_url)

Your fixture runs at the module level. So all tests in one file would use the same password and remove it??

So what you need is this:

Fixture that runs again for each test

Fixture that returns some random repo URL and random password for testing, as we do in some places: test_repo_url = f'vorta-test-repo.{uuid.uuid4()}.com:repo' # Random repo URL to avoid macOS keychain and password = str(uuid.uuid4())

The repo URL and password are then used during testing

After each test function the password for the test repo is removed again.

This should be more clear and deduplicates some more code (creating a random repo URL and password)

One more thing: vorta-test-repo.{uuid.uuid4()}.com:repo would give a SFTP-style URL that's not supported in the future. Best if you change it to ssh:// format.

This means that the repo url and password that are generated in the test has to be removed and only to be generated in the fixture and then used in multiple tests . Same question for remove_password ?

Mar 25 '23 13:03 ganeshrevadi

Repo URL and password are generated by the fixture. The password manager entry is also removed by the fixture during cleanup.

Mar 25 '23 14:03 m3nu

@m3nu I've tried implementing pytest fixture as per your suggestion , but the tests are failing. Need some help here!

Mar 25 '23 18:03 ganeshrevadi

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

Jul 19 '23 06:07 stale[bot]

Are you still working on this @ganeshrevadi? This would be a big quality of life improvement for developers.

Jul 21 '23 14:07 real-yfprojects

@real-yfprojects Hey I'm currently not working on this!

Jul 21 '23 14:07 ganeshrevadi