Closed gfyoung closed 6 years ago
Well, how cool is that? Thank you @gfyoung! I dig this.
May I ask for one more thing? Can you document this test suite, however briefly, in readme_template.md?
@StevenBlack : Sure thing! Quick question : these tests use the nosetests
paradigm, though apparently the cool thing is to use pytest
nowadays. Any preferences? nosetests
was easier to setup (especially with mock
), but I could try to move it to pytest
(in another PR of course) if there's a preference.
@gfyoung no preference here. I suppose my standard answer should be, which of the two methods has the brighter future, in your view?
Several things about updateHostsFile.py bother me, so if this means revisiting its structure to make it more testable, I'm pumped.
@StevenBlack : The current trend is moving away from nosetests
and unittest
towards pytest
. pytest
does help to simplify testing and provides a lot of useful decorators for testing. That being said, I'm hesitant about how well it would do on tests like these that very much rely on mocking a lot of different system and architecture-level checks so that we can cover a lot of ground.
I'll certainly give it a shot, and if it does help to make the tests cleaner, then sure, but I'll leave that for another time. I'd like to look at the structure of updateHostsFile.py
first before I revisit this.
Also of importance is checking the regex to make sure it properly matches to the domains and host file entries that we intend. That was part of the reason why I wrote this (xref #338 and #341).
I agree with everything above.
UpdateHostsFile.py started as a simple naïve merge-and-deduplication of several hosts files. Then contributions came from every corner, and features got added, one-at-a-time.
As they say, sooner or later you WILL do a proper requirements analysis. But in many cases, this included, life comes at you fast. New ideas and features appear opportunistically, so evolution is organic.
So introducing testing provides impetus to do it right.
@StevenBlack : For reference, there is some logic behind the trend:
"Nose has been in maintenance mode for the past several years and will likely cease without a new person/team to take over maintainership." (from main page here).
Thus, there is some motivation to try to write it using alternatives like nose2
and pytest
(or just plain unittest
), though the latter seems to have gained traction.
@StevenBlack : readme_template.md
content has been added. Ready to merge if there's nothing else.
@gfyoung when I use python
as it says in the docs, I get the following.
$ python testUpdateHostsFile.py
Traceback (most recent call last):
File "testUpdateHostsFile.py", line 28, in <module>
import mock
ImportError: No module named mock
However I get this when I use Python 3, so it works under Python 3.
$ python3 testUpdateHostsFile.py
...................................................
----------------------------------------------------------------------
Ran 51 tests in 0.017s
Do we have a Python 3 constraint here?
@StevenBlack : Oops. Need to add that to the documentation. Just run
pip install mock
Then the tests will run fine (note: I run tests both on Python 2 and 3 on Travis).
pip install mock
works fine, but thereafter I get the same error.
$ python testUpdateHostsFile.py
Traceback (most recent call last):
File "testUpdateHostsFile.py", line 28, in <module>
import mock
ImportError: No module named mock
Which makes no intrinsic sense. However, that's what I get.
@StevenBlack : Are you sure you're pointing at the right pip
? It might have just installed it for Python 3 (which I suspect is your default).
@gfyoung
$ which pip
/usr/local/bin/pip
@StevenBlack : Try pip --version
@gfyoung
$ pip --version
pip 9.0.1 from /usr/local/lib/python2.7/site-packages (python 2.7)
Can you go into site-packages
and see if there is a mock
package installed?
Also, can you try python -V
?
@StevenBlack : Added instructions for installing mock
in the README. Let me know if you can resolve your installation issues on your own machine. Otherwise, this should be good to merge.
@gfyoung it installed and ran on my secondary Mac.
Seems OK, though I did get the following tree errors under Mac OS latest.
$ python testUpdateHostsFile.py
...........................................E..EE...
======================================================================
ERROR: test_no_isatty (__main__.TestSupportsColor)
----------------------------------------------------------------------
Traceback (most recent call last):
File "testUpdateHostsFile.py", line 709, in test_no_isatty
with self.mock_property("sys.stdout.isatty") as obj:
File "/usr/local/lib/python2.7/site-packages/mock/mock.py", line 1460, in __enter__
setattr(self.target, self.attribute, new_attr)
AttributeError: 'file' object attribute 'isatty' is read-only
======================================================================
ERROR: test_posix (__main__.TestSupportsColor)
----------------------------------------------------------------------
Traceback (most recent call last):
File "testUpdateHostsFile.py", line 668, in test_posix
with self.mock_property("sys.stdout.isatty") as obj:
File "/usr/local/lib/python2.7/site-packages/mock/mock.py", line 1460, in __enter__
setattr(self.target, self.attribute, new_attr)
AttributeError: 'file' object attribute 'isatty' is read-only
======================================================================
ERROR: test_windows_ansicon (__main__.TestSupportsColor)
----------------------------------------------------------------------
Traceback (most recent call last):
File "testUpdateHostsFile.py", line 693, in test_windows_ansicon
with self.mock_property("sys.stdout.isatty") as obj:
File "/usr/local/lib/python2.7/site-packages/mock/mock.py", line 1460, in __enter__
setattr(self.target, self.attribute, new_attr)
AttributeError: 'file' object attribute 'isatty' is read-only
----------------------------------------------------------------------
Ran 51 tests in 0.016s
@StevenBlack : Ah, this is good to know. Should add an OSX build on Travis.
@StevenBlack : Hmm...I guess my work is needed before OSX can be added. Will work out in a follow-up.
@gfyoung Thanks. I applied abb02b7 and I still see those three errors.
It feels close though! Thanks for taking care of this.
@StevenBlack : The commit was to add OSX to Travis. I actually didn't do anything code-wise.
@StevenBlack : Could you pull down my branch again and give it a shot?
(FYI, I tried it on my own Mac, and I didn't get this error...hmmm)
Beauty!
Merging 😎
@StevenBlack : Thanks! As an immediate follow-up, will attempt to load OSX onto Travis.
Been sometime since I last contributed!
This PR adds a whole suite of tests for
updateHostsFile.py
. I tested as much as I could, though the lack of modularity in the script made some functions hard to test.