-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
bpo-28334: fix netrc not working when $HOME is not set #123
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import netrc, os, unittest, sys, tempfile, textwrap | ||
from unittest import mock | ||
from test import support | ||
|
||
|
||
|
@@ -126,8 +127,44 @@ def test_security(self): | |
os.chmod(fn, 0o622) | ||
self.assertRaises(netrc.NetrcParseError, netrc.netrc) | ||
|
||
def test_main(): | ||
support.run_unittest(NetrcTestCase) | ||
def test_file_not_found_in_home(self): | ||
d = support.TESTFN | ||
os.mkdir(d) | ||
self.addCleanup(support.rmtree, d) | ||
with support.EnvironmentVarGuard() as environ: | ||
environ.set('HOME', d) | ||
self.assertRaises(FileNotFoundError, netrc.netrc) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you explicitly pass a not exist file, it could also raise the error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. I wrote an other test for this. (I like to follow the 'one assert per test' rule) |
||
def test_file_not_found_explicit(self): | ||
self.assertRaises(FileNotFoundError, netrc.netrc, | ||
file='unlikely_netrc') | ||
|
||
def test_home_not_set(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I don't understand how this test is related to the case when HOME is not set. Why not use the simple test suggested by @berkerpeksag above? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My suggested test won't work, because of the following code in if 'HOME' not in os.environ:
import pwd
userhome = pwd.getpwuid(os.getuid()).pw_dir I have a def test_foo(self):
with support.EnvironmentVarGuard() as environ:
environ.unset('HOME')
self.assertRaises(FileNotFoundError, netrc.netrc)
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, now I see that this is the simplest platform-independent test. |
||
fake_home = support.TESTFN | ||
os.mkdir(fake_home) | ||
self.addCleanup(support.rmtree, fake_home) | ||
fake_netrc_path = os.path.join(fake_home, '.netrc') | ||
with open(fake_netrc_path, 'w') as f: | ||
f.write('machine foo.domain.com login bar password pass') | ||
os.chmod(fake_netrc_path, 0o600) | ||
|
||
orig_expanduser = os.path.expanduser | ||
called = [] | ||
|
||
def fake_expanduser(s): | ||
called.append(s) | ||
with support.EnvironmentVarGuard() as environ: | ||
environ.set('HOME', fake_home) | ||
result = orig_expanduser(s) | ||
return result | ||
|
||
with support.swap_attr(os.path, 'expanduser', fake_expanduser): | ||
nrc = netrc.netrc() | ||
login, account, password = nrc.authenticators('foo.domain.com') | ||
self.assertEqual(login, 'bar') | ||
|
||
self.assertTrue(called) | ||
|
||
|
||
if __name__ == "__main__": | ||
test_main() | ||
unittest.main() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Use :func:`os.path.expanduser` to find the ``~/.netrc`` file in | ||
:class:`netrc.netrc`. If the file does not exist, a | ||
:exc:`FileNotFoundError` is raised. Patch by Dimitri Merejkowsky. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't test a new behavior. Needed a test for the case when HOME is not set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does. It checks the type of the exception raised, which is the whole point of this patch,
getting rid of the weird 'OSError' about HOME not set ...
Yeah I was not sure about this one. I've added a test where I monkeypatch
os.path.expanduser
, but I'm not sure it's useful. My guts tell me such a test is too close to the implementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Serhiy is right that we need a test for this case and unless I'm missing something you can do it without doing any mocking: