Skip to content

Use plaintext keyring backend in test#7209

Merged
yarikoptic merged 1 commit into
datalad:maintfrom
bpoldrack:fix-6623
Dec 10, 2022
Merged

Use plaintext keyring backend in test#7209
yarikoptic merged 1 commit into
datalad:maintfrom
bpoldrack:fix-6623

Conversation

@bpoldrack
Copy link
Copy Markdown
Member

Trying to fix #6623 and possibly #7165 by using MemoryKeyring with this test.

@yarikoptic Provided CI is happy to begin with, could you please give it a try?

@bpoldrack bpoldrack added the semver-tests Changes only affect tests, no impact on version label Nov 30, 2022
@bpoldrack bpoldrack marked this pull request as draft November 30, 2022 14:32
@yarikoptic
Copy link
Copy Markdown
Member

CI seems to be very unhappy

=================================== FAILURES ===================================
________________________ test_datalad_credential_helper ________________________
[gw0] linux -- Python 3.8.6 /tmp/dl-miniconda-fuc1g7_4/bin/python
args = (), kwargs = {}
keyring = <datalad.support.keyring_.MemoryKeyring object at 0x7f8fd21108e0>
    @wraps(t)
    @attr('with_memory_keyring')
    def  _wrap_with_memory_keyring(*args, **kwargs):
        keyring = MemoryKeyring()
        with patch("datalad.downloaders.credentials.keyring_", keyring):
>           return t(*(args + (keyring,)), **kwargs)
../datalad/tests/utils_pytest.py:900: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
arg = (<datalad.support.keyring_.MemoryKeyring object at 0x7f8fd21108e0>,)
kw = {}, filename = '/tmp/datalad_temp_test_datalad_credential_helpermn5b_1xa'
    @wraps(t)
    def  _wrap_with_tempfile(*arg, **kw):
        if 'dir' not in tkwargs.keys():
            # if not specified otherwise, respect datalad.tests.temp.dir config
            # as this is a test helper
            tkwargs['dir'] = dl_cfg.get("datalad.tests.temp.dir")
        with make_tempfile(wrapped=t, **tkwargs) as filename:
>           return t(*(arg + (filename,)), **kw)
E           TypeError: test_datalad_credential_helper() takes from 0 to 1 positional arguments but 2 were given
../datalad/tests/utils_pytest.py:954: TypeError

works for you locally?

@bpoldrack
Copy link
Copy Markdown
Member Author

@yarikoptic

works for you locally?

No. Didn't try b/c I didn't have the interactive issue to begin with.
However, fixed use of decorator (missed it does not only patch but aslo pass the keyring on) locally, but then I get the same failure as in #7165. Problem ATM is, that the MemoryKeyring is used in the python context of the test, but what stores things to a keyring is the git-credential-helper in a subprocess - patch doesn't apply.

@bpoldrack
Copy link
Copy Markdown
Member Author

I'll try to set the config file for keyring in the tests env to declare PlaintextKeyring the default, so it wouldn't be encrypted and therefore no need to ask interactively when creating it.

@bpoldrack
Copy link
Copy Markdown
Member Author

Ok, different approach, @yarikoptic : Setting the to be used backend to PlaintextKeyring via env var, so it's not encrypted and doesn't ask for password on creation. As far as I can tell keyring figuring the paths to use accounts correctly for the fake HOME. Setting it "fixed" this way shouldn't conflict with tests setting MemoryKeyring or other, because that is happening at runtime and at our wrapping layer.
WDYT?

@yarikoptic
Copy link
Copy Markdown
Member

sounds good, especially because apparently that is what I ended up doing in debian package:

❯ git grep PYTHON_KEYRING_BACKEND
debian/rules:export PYTHON_KEYRING_BACKEND=keyrings.alt.file.PlaintextKeyring
debian/rules:       PYTHON_KEYRING_BACKEND=keyrings.alt.file.PlaintextKeyring PATH=$(CURDIR)/bin:$$PATH \

❯ git log -SPYTHON_KEYRING_BACKEND debian/rules
commit c280fe2646e3f0dd200507c3c295b13d6e800a1f
Author: Yaroslav Halchenko <debian@onerussian.com>
Date:   Fri Jul 8 18:37:16 2022 -0400

    Various tune ups for build to succeed with testing

by patching `os.environ` from within `conftest.py:setup_package` in
order to not have the keyring asking for a password in order to be
created in non-interactive (CI-) test runs.

(Closes datalad#6623)
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 8, 2022

Codecov Report

Base: 88.89% // Head: 90.92% // Increases project coverage by +2.02% 🎉

Coverage data is based on head (d8d538a) compared to base (c62151b).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##            maint    #7209      +/-   ##
==========================================
+ Coverage   88.89%   90.92%   +2.02%     
==========================================
  Files         355      355              
  Lines       46674    46695      +21     
  Branches     6351     6355       +4     
==========================================
+ Hits        41493    42456     +963     
+ Misses       5166     4224     -942     
  Partials       15       15              
Impacted Files Coverage Δ
datalad/conftest.py 93.69% <100.00%> (+0.05%) ⬆️
datalad/distribution/utils.py 94.00% <0.00%> (-6.00%) ⬇️
datalad/local/remove.py 95.52% <0.00%> (-1.50%) ⬇️
datalad/interface/utils.py 95.25% <0.00%> (-0.73%) ⬇️
datalad/interface/base.py 94.84% <0.00%> (-0.52%) ⬇️
datalad/cli/parser.py 86.03% <0.00%> (-0.46%) ⬇️
datalad/core/local/run.py 95.83% <0.00%> (-0.33%) ⬇️
datalad/config.py 97.05% <0.00%> (-0.23%) ⬇️
datalad/support/annexrepo.py 90.56% <0.00%> (-0.09%) ⬇️
datalad/support/network.py 90.10% <0.00%> (-0.07%) ⬇️
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bpoldrack
Copy link
Copy Markdown
Member Author

@yarikoptic :

Redone, env var is now patched in setup_package instead of needing to be provided. Should fix #6623 and possibly make your debian patch superfluous. Can you confirm, that #7165 also is addressed?

IIRC, @TheChymera had to deal with that issue as well, right?

@bpoldrack bpoldrack marked this pull request as ready for review December 8, 2022 15:51
@yarikoptic
Copy link
Copy Markdown
Member

confirming success -- thank you @bpoldrack !

(dev3) yoh@typhon:~/proj/datalad/datalad$ python -m pytest -s -v -k test_datalad_credential_helper --tb=short datalad
====================================================== test session starts ======================================================
platform linux -- Python 3.9.2, pytest-7.2.0, pluggy-1.0.0 -- /home/yoh/proj/datalad/datalad/venvs/dev3/bin/python
cachedir: .pytest_cache
rootdir: /home/yoh/proj/datalad/datalad, configfile: tox.ini
plugins: cov-4.0.0, fail-slow-0.3.0, xdist-3.0.2
collected 1319 items / 1318 deselected / 1 selected                                                                             

datalad/local/tests/test_gitcredential.py::test_datalad_credential_helper create(ok): . (dataset)
PASSEDVersions: annexremote=1.6.0 boto=2.49.0 cmd:7z=16.02 cmd:annex=10.20221103+git83-g31fc11771-1~ndall+1 cmd:bundled-git=UNKNOWN cmd:git=2.30.2 cmd:ssh=8.4p1 cmd:system-git=2.30.2 cmd:system-ssh=8.4p1 datalad=0.17.9+70.gd8d538ae7 exifread=3.0.0 humanize=4.4.0 iso8601=1.1.0 keyring=23.11.0 keyrings.alt=UNKNOWN msgpack=1.0.4 mutagen=1.46.0 patoolib=1.12 platformdirs=2.5.3 requests=2.28.1 tqdm=4.64.1
Obscure filename: str=b' |;&%b5{}\'"<>\xce\x94\xd0\x99\xd7\xa7\xd9\x85\xe0\xb9\x97\xe3\x81\x82 .datc ' repr=' |;&%b5{}\'"<>ΔЙקم๗ あ .datc '
Encodings: default='utf-8' filesystem='utf-8' locale.prefered='UTF-8'
Environment: LANG='en_US.UTF-8' PATH='/home/yoh/proj/datalad/datalad/venvs/dev3/bin:/home/yoh/git-annexes/10.20221103+git83-g31fc11771/usr/lib/git-annex.linux:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games:/usr/local/cuda/bin' GIT_CONFIG_PARAMETERS="'init.defaultBranch=dl-test-branch' 'clone.defaultRemoteName=dl-test-remote'" PYTHON_KEYRING_BACKEND='keyrings.alt.file.PlaintextKeyring' GIT_ASKPASS='true'


============================================== 1 passed, 1318 deselected in 2.37s ===============================================
(dev3) yoh@typhon:~/proj/datalad/datalad$ 

@yarikoptic-gitmate
Copy link
Copy Markdown
Collaborator

PR released in 0.17.10

@mih mih changed the title Use MemoryKeyring in test Use plaintext keyring backend in test Feb 27, 2023
@mih
Copy link
Copy Markdown
Member

mih commented Feb 27, 2023

I have adjusted the PR title to match its actual content

@mih mih deleted the fix-6623 branch February 27, 2023 12:29
mih added a commit to mih/datalad-next that referenced this pull request Feb 27, 2023
It runs unconditionally and verifies that no test leave a keyring
(credential store) modification behind.

The fixture assums the present datalad-core keyring setup, which
configures a plaintext backend for all tests
datalad/datalad#7209

Ping datalad/datalad#7297
yarikoptic added a commit to yarikoptic/datalad that referenced this pull request Mar 28, 2023
…to be used during tests

We hardcoded to set it to a plain text keyring in
d8d538a of
datalad#7209 to completely disable possible
attempt of keyring to interact with user e.g. to establish a new keyring.
That was done to address datalad#6623 which came up during debian package building.

But the need for older behavior was realized while approaching datalad#7340
where interactions with s3 were replaced to use boto3 but to test I needed
credentials, but we were just skipping the tests since credentials were not
present in that fake keyring.

With this patch I could run

  DATALAD_TESTS_CREDENTIALS=system python3 -m pytest -s -v datalad/downloaders/tests/test_s3.py

and see tests to interact with S3.  Test relating to NDA were failing
since most likely NDA broke the "flow" of how to access data there.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-tests Changes only affect tests, no impact on version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test_datalad_credential_helper test wants interaction

4 participants