Agent test test_confirm may fail

[vincentbernat]

Hey!

I have the following failure during the build of asyncssh Debian package:

======================================================================
ERROR: test_confirm (tests.test_agent._TestAPI)
Test confirmation of key
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/build/python-asyncssh-1.8.1/tests/util.py", line 59, in async_wrapper
    return self.loop.run_until_complete(wrapped_func)
  File "/usr/lib/python3.5/asyncio/base_events.py", line 466, in run_until_complete
    return future.result()
  File "/usr/lib/python3.5/asyncio/futures.py", line 293, in result
    raise self._exception
  File "/usr/lib/python3.5/asyncio/tasks.py", line 239, in _step
    result = coro.send(None)
  File "/build/python-asyncssh-1.8.1/tests/test_agent.py", line 40, in agent_wrapper
    yield from asyncio.coroutine(func)(self, agent)
  File "/build/python-asyncssh-1.8.1/tests/test_agent.py", line 278, in test_confirm
    sig = yield from agent_key.sign(b'test')
  File "/build/python-asyncssh-1.8.1/asyncssh/agent.py", line 148, in sign
    data, self._flags))
  File "/build/python-asyncssh-1.8.1/asyncssh/agent.py", line 272, in sign
    raise ValueError('Unable to sign with requested key')
ValueError: Unable to sign with requested key

----------------------------------------------------------------------

I can reproduce by running python3.5 setup.py test manually while python3.5 unittest discover -v doesn't trigger the failure. If I run manually python3.5 setup.py test, I get a prompt for SSH agent. So, it's like it doesn't use SSH_ASKPASS environment variable for some reason. After looking a bit, I have traced this to commit 6188a4c. I have tried to find which variable could be missing, but couldn't find it.

[vincentbernat]

I have replaced the environment stuff by:

        os.environ['LOGNAME'] = 'guest'
        os.environ['HOME'] = '.'
        if 'DISPLAY' in os.environ:
            del os.environ['DISPLAY']
        if 'XAUTHORITY' in os.environ:
            del os.environ['XAUTHORITY']

Works for me.

[ronf]

Hmm. The intent of that commit you referenced was to avoid unpredictable values coming from the user's environment from impacting the test (such as DISPLAY and XAUTHORITY). Also, the code in test_agent.py doesn't reference the ServerTestCase class at all. It directly uses AsyncTestCase. So, I'm not sure how any of the code in the commit you mentioned would even be getting involved when running the tests in test_agent.py.

Since I'm not seeing this failure, I'm guessing it's some kind of race. Are you running a test framework where multiple of the tests might be running in parallel, such that two different tests are trying to both modify the shared os.environ?

[vincentbernat]

There is no race. If, for example, test_channels.py is run before test_agent.py, the environment is emptied (same process). For some reason, in this case, ssh-agent won't work as expected.

[ronf]

Ok - I'm not seeing this failure when I run the full set of unit tests in the default order, but if I manually specify that it should run the test_channel.py tests before the test_agent.py tests, I can reproduce the failure in test_agent._TestAPI.test_confirm. Thanks for the tip -- I'll take a closer look.

[ronf]

I don't have an explanation for this yet, but it seems like replacing os.environ with a regular dictionary causes some kind of strange failure in the environment Python provides to spawned processes. If I replace:

        os.environ = {'LOGNAME': 'guest', 'HOME': '.'}

with:

        os.environ.clear()
        os.environ['LOGNAME'] = 'guest'
        os.environ['HOME'] = '.'

I no longer see the failure, with either ordering of the tests. @vincentbernat , can you confirm this?

[vincentbernat]

That makes sense: if you assign a dict to os.environ, you are just replacing the object with a dict and the object doesn't have a chance to update the environment variables. With your chance, I no longer see the failure either.

[ronf]

Yeah, I guess the issue here is that the current code is no longer able to pass in DISPLAY or SSH_ASKPASS properly to ssh-agent after os.environ is changed this way, but it's only seen if a test based on ServerTestCase runs before these agent tests.

Thanks for the report! I'll get this change in soon.

[ronf]

This fix is now available in this 1.9.0 release.