Skip to content

Comments

when "disabled", keyring uses a "null" keyring and should be unavaila…#5251

Merged
radoering merged 6 commits intopython-poetry:masterfrom
lakinwecker:master
Apr 12, 2022
Merged

when "disabled", keyring uses a "null" keyring and should be unavaila…#5251
radoering merged 6 commits intopython-poetry:masterfrom
lakinwecker:master

Conversation

@lakinwecker
Copy link
Contributor

@lakinwecker lakinwecker commented Feb 28, 2022 •
edited
Loading

…ble.

Pull Request Check List

Resolves: #5250

  • Added tests for changed code.
  • Updated documentation for changed code.

I didn't see any tests or docs for this part of the code, happy to update them as required.

@abn
Copy link
Member

abn commented Mar 5, 2022

@lakinwecker can you add some regression test coverage for this?

@lakinwecker
Copy link
Contributor Author

lakinwecker commented Mar 12, 2022

@abn I looked for tests related to this stuff and failed to find any. Are there any other unit tests that do things similarily that I can follow to make tests? If so, I can try and add them, yes.

@radoering
Copy link
Member

radoering commented Mar 20, 2022

@lakinwecker Tests are in test_password_manager.py. Probably, you should add two tests similar to

poetry/tests/utils/test_password_manager.py

Lines 190 to 195 in 15f6ec1

def test_keyring_with_chainer_backend_and_not_compatible_only_should_be_unavailable(
with_chained_keyring: None,
):
key_ring = KeyRing("poetry")
assert not key_ring.is_available()

one with a simple null and one with a chained null. (And maybe you should rename the fixture with_chained_keyring to with_chained_fail_keyring so you can add with_chained_null_keyring). The fixtures are here:

poetry/tests/conftest.py

Lines 109 to 127 in 15f6ec1

@pytest.fixture()
def with_fail_keyring() -> None:
import keyring
from keyring.backends.fail import Keyring
keyring.set_keyring(Keyring())
@pytest.fixture()
def with_chained_keyring(mocker: MockerFixture) -> None:
from keyring.backends.fail import Keyring
mocker.patch("keyring.backend.get_all_keyring", [Keyring()])
import keyring
from keyring.backends.chainer import ChainerBackend
keyring.set_keyring(ChainerBackend())

@lakinwecker
Copy link
Contributor Author

lakinwecker commented Apr 11, 2022

test_keyring_with_chainer_backend_and_not_compatible_only_should_be_unavailable doesn't fail when you mess with the "fail" code that I modified. So I'm not convinced this test properly exercises it.

@lakinwecker
Copy link
Contributor Author

lakinwecker commented Apr 11, 2022

Alright, the mocks were wrong for those chainer tests. I fixed them and now the tests fail if I change the name of the 'bad' keyrings in the password_manager.py checks. So, I think this adds to tests to properly exercise it.

Is there documentation I should also update?

@lakinwecker
Copy link
Contributor Author

lakinwecker commented Apr 11, 2022

I fixed the formatting issues, I'm happy to squash the commits if you prefer. Otherwise let me know if there is anything else you need for this to be reviewed.

@radoering radoering merged commit f462b7f into python-poetry:master Apr 12, 2022
@kasteph kasteph mentioned this pull request May 30, 2022
Merged
@lakinwecker lakinwecker mentioned this pull request May 3, 2023
Closed
3 tasks
@github-actions
Copy link

github-actions bot commented Feb 29, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@radoering radoering radoering approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

When keyring is disabled, poetry tries to use it anyways.

3 participants

@lakinwecker @abn @radoering