#14 Add tests. pytest_watch.command and pytest_watch.watcher #79

apast · 2018-01-03T16:15:38Z

Hi,

This PR refers to #14 .

I added initial tests over command.py (coverage 98%) and to main.py.

Tests are implemented using basic unittest, and I run it on Linux using python 2 & 3.

I am implementing it from top-down, before try to add or change any funcionality.

General structure of code was keep as the same. I only fixed docopt [default: ...] definitions for --spool and --ext and now we have default values defined by documentation and being set by docopt parser. I updated README.md accordingly.

I mocked pytest_watch.watcher.watch and pytest_watch.config.merge_config to isolate and focus on CLI input and validadion.

Can you review and give me some feedback to improve the code before accept this PR?

Thank you!

Now, default value is set as 200.

…fault value as 200

joeyespo · 2018-01-04T03:56:55Z

pytest_watch/command.py

@@ -29,7 +29,7 @@
  --pdb                 Start the interactive Python debugger on errors.
                        This also enables --wait to prevent pdb interruption.
  --spool <delay>       Re-run after a delay (in milliseconds), allowing for
-                        more file system events to queue up (default: 200 ms).
+                        more file system events to queue up [default: 200].
  -p --poll             Use polling instead of OS events (useful in VMs).


Ah, nice removal of the redundant ms.

joeyespo · 2018-01-04T04:00:13Z

pytest_watch/command.py

@@ -11,7 +11,7 @@
  --ignore <dir>        Ignore directory from being watched and during
                        collection (multi-allowed).
  --ext <exts>          Comma-separated list of file extensions that can
-                        trigger a new test run when changed (default: .py).
+                        trigger a new test run when changed [default: .py].


~~Are brackets a common for default values in CLI tool --help texts?~~ Nevermind. Just noticed docopt uses brackets in their docs. Thanks for updating 👍

joeyespo · 2018-01-04T04:04:05Z

pytest_watch/__main__.py

    sys.path.append(os.path.dirname(__file__))

    from pytest_watch.command import main
-    main()
+    main(argv=sys.argv[1:])


I think main() with no args is more aesthetic. The default is to use argv[1:] and adding that here adds noise.

Could you change it back?

If your rationale was "I couldn't tell what main's default were", a better docstring for main could address that instead of more code.

joeyespo · 2018-01-04T04:12:06Z

pytest_watch/command.py

    if args['--ext'] == '*':
        extensions = ALL_EXTENSIONS
    elif args['--ext']:
        extensions = [('.' if not e.startswith('.') else '') + e
                      for e in args['--ext'].split(',')]
-    else:
-        extensions = None


Ahh I see fixing the default value in docopt allows you to remove this. Very nice 👍

joeyespo · 2018-01-04T04:24:15Z

pytest_watch/command.py

+    try:
+        spool = int(spool)
+    except ValueError:
+        sys.stderr.write('Error: Spool (--spool %s) must be an integer.\n' % spool)


Two things.

I used to willfully break PEP8's "80 character limit" rule all the time. With side-by-side comparisons and 3-way merges, I've since come to appreciate this style rule. Could you do the same here?

I thought I read a while back that Python favored .format over % these days. A quick Google search yields no results on this. But since I've since switched to using .format everywhere, could you do the same here for consistency? A case could be made later to go back to % everywhere.

This should do it:

sys.stderr.write( 'Error: Spool (--spool {}) must be an integer.\n'.format(spool))

Or perhaps:

print( 'Error: Spool (--spool {}) must be an integer.\n'.format(spool), file=sys.stderr)

joeyespo · 2018-01-04T04:25:24Z

pytest_watch/command.py

+
+    if spool < 0:
+        sys.stderr.write("Error: Spool value(--spool %s) must be positive integer" % spool)
+        return 2


Ahh nice catch! 👍

Also, same comment as above about using 80 characters.

joeyespo · 2018-01-04T04:26:17Z

pytest_watch/tests/test_main.py

+from pytest_watch.__main__ import run_cli
+
+
+class TestRunCLI(unittest.TestCase):


Is there any chance you could switch to classless pytest? Using classes just adds overhead IMHO.

If not, I'll probably go back and do this at a (much) later date.

Yes, I will adopt this classless where it is unecessary.

joeyespo · 2018-01-04T04:28:54Z

pytest_watch/watcher.py

+    ignore = [] if ignore is None else ignore
+    extensions = [] if extensions is None else extensions
+    pytest_args = [] if pytest_args is None else pytest_args
+


This is much better than what I had 👍

Avoiding any accidental mutation is definitely a good thing. Good eye!

joeyespo · 2018-01-04T04:54:28Z

pytest_watch/tests/test_command.py

@@ -0,0 +1,437 @@
+import sys


(Same comment in this file about using functions over class-based tests.)

Also assert x != y is much more readable than self.assertFalse(condition), etc. Pytest does an amazing job of inspecting values when things go wrong, so I'm all for the boost in readability using raw assert. Same goes for assert x in y over self.assertIn(x, y) and assert x not in y over self.assertNotIn(x, y)

And again, if you'd rather get this in now, that's fine. I'll probably go back and switch it later on if that's the case.

Perfect. I agree with readability os simple assert. I will adopt this for general assertions.

I am in doubt about self.assertListEqual and self.assertDictEqual calls. I think it is better to use unittest helpers in this case. What do you think?

Thanks

Well given that pytest advocates the use of assert over self.assert, and this is a pytest-based project, I think using vanilla assert is appropriate.

Here's the top bullet from pytest's Features list:

Detailed info on failing assert statements (no need to remember self.assert* names)

FWIW readability and the lack of boilerplate was what attracted me to pytest to begin with.

joeyespo · 2018-01-04T04:58:02Z

pytest_watch/command.py

@@ -87,23 +91,24 @@ def main(argv=None):
    if args['--pdb']:
        pytest_args.append('--pdb')

-    # Parse extensions
+    # Parse extensions [default: .py]


Nice addition of comments 👍 Even more so with the defaults right there.

joeyespo · 2018-01-04T04:58:06Z

.gitignore

@@ -14,3 +14,7 @@ env
 .DS_Store
 Desktop.ini
 Thumbs.db
+.coverage
+.coverage.*
+venv


"Environment file" may be a better place for venv.

I wonder if it'd make sense to add a new "Testing files" for things like .coverage.

About the suggested github .gitignore (https://github.com/github/gitignore/blob/master/Python.gitignore), it is up to date and a more complete version for python related projects.

Can we replace current version by the community version?

joeyespo

This looks great! Thank you 😄

Only requested some small change here and there. I think we can get this in quickly after that.

joeyespo · 2018-01-04T05:14:25Z

Nice! Just tested on Windows and everything is passing 💯 😌

The only thing I had to do was install pytest-cov. I think we can signal that requirement by:

Adding a new requirements-test.txt file
Adding pytest-cov>=2.5.1 to it
Adding a tests_require field to setup.py:
```
tests_require=read('requirements-test.txt').splitlines()
```
(Note that after doing this, running python setup.py test will succeed even if you don't manually install pytest-cov for pytest to use. This creates a local .eggs directory. May be worth adding that as an entry to .gitignore.)

joeyespo · 2018-01-04T05:14:40Z

@blueyed Anything you'd like to add?

apast · 2018-01-04T10:53:55Z

Hello, Joe,

Thanks for your detailed review.

I will check this and change code or reply properly.

[]'s,

Andre

…ted in joeyespo#79 (review), it was preferable to add more detailed docstring.

Messages formatting using .format() methods Tests over error messages for invalid --spool arguments Related to: joeyespo#79 (review)

apast · 2018-01-04T16:06:38Z

Joe,

it seems excellent to add this requirements-test.txt file and declare it in setup.py file.

I will implement this suggestion and commit soon.

[]'s,
Andre

apast · 2018-01-04T17:12:20Z

I added a version checker inside setup.py for python 2 compatibility.

I really don't know if it is the best pratice, or can we keep it this way for this moment. What do you think?

[]'s,

… ignored resources

…ework

… into setup.py extras_require was added to config mock module conditional installation was moved to extras_require section, simplifying duplicated verification

…ttern. Initial usage of conftest.py and fixtures

…e|False) cases and legacy python versions

…llback

…ere are issues when they are on environment

… doesnt support symlinks

suggiro added 6 commits December 31, 2017 11:01

Fix default value syntax for docopt entry for 'spool' parameter.

755e402

Now, default value is set as 200.

Add initial tests for command line arguments.

dc82b40

git ignore coverage directory

8e63df5

simple comment on pytest config argument

314dc78

Added 'spool' parameter validation and coverage tests, considering de…

a3286f9

…fault value as 200

Added 'spool' parameter validation and coverage tests, considering de…

8b64901

…fault value as 200

apast changed the title ~~14 add tests~~ Add CLI test coverage (98%) Jan 3, 2018

apast changed the title ~~Add CLI test coverage (98%)~~ Add CLI test coverage (pytest_watch.command: 98%) Jan 4, 2018

joeyespo reviewed Jan 4, 2018

View reviewed changes

joeyespo requested changes Jan 4, 2018

View reviewed changes

apast pushed a commit to apast/pytest-watch that referenced this pull request Jan 4, 2018

Replace explicit sys.argv[1:] declaration from main() call. As sugges…

39277a3

…ted in joeyespo#79 (review), it was preferable to add more detailed docstring.

apast changed the title ~~Add CLI test coverage (pytest_watch.command: 98%)~~ [WIP] Add CLI test coverage (pytest_watch.command: 98%) Jan 4, 2018

apast changed the title ~~[WIP] Add CLI test coverage (pytest_watch.command: 98%)~~ Add CLI test coverage (pytest_watch.command: 98%) Jan 4, 2018

apast pushed a commit to apast/pytest-watch that referenced this pull request Jan 4, 2018

PEP8 line length limits applied to stderr messages

2fe1231

Messages formatting using .format() methods Tests over error messages for invalid --spool arguments Related to: joeyespo#79 (review)

apast changed the title ~~Add CLI test coverage (pytest_watch.command: 98%)~~ Add CLI test coverage over pytest_watch.command Jan 4, 2018

suggiro and others added 29 commits May 20, 2018 21:46

Reverting .gitignore to lean version containing only project inherent…

0be301a

… ignored resources

Initial clean up for unittest.TestCase and other dependencies of fram…

6f1638c

…ework

requirements.txt and requirements-test.txt dependencies were inserted…

81f5c63

… into setup.py extras_require was added to config mock module conditional installation was moved to extras_require section, simplifying duplicated verification

Move default pytest_watcher.watch mock to pytest-mock mocker usage pa…

3849636

…ttern. Initial usage of conftest.py and fixtures

Added pytest-mock dependency to setup.py & requirements-test.txt

2e250cb

Added DEPS_TESTING to DEPS_QA

8d699d5

Added codecov.com support to Travis-CI

77e529b

Add -v verbosity to travis-ci test coverage

47adfc3

split summary output definitions from core watcher module.

bbe37af

For tests/mock purposes, ctype was moved to module level import section.

1f597c8

Added tests for helpers.send_keyboard_event, covering is_windows (Tru…

e39858e

…e|False) cases and legacy python versions

Compatibility with 3- versions. Format string was introduced. Now, ro…

3c8ff1b

…llback

Fix test_helpers for python 2.7 compatibility and some simplifications

1f8888f

Fix test_helpers for python 2.7 compatibility and some simplifications

d5eefb9

pep8 updates

3932987

Remove conflicting pdbpp and pytest-pdb. It should be analysed why th…

f62cd37

…ere are issues when they are on environment

Added helpers.clear and helpers.beep tests & fixtures

1f11440

Added pytest_watch.util test, covering Exception handling

828c6eb

Added pytest_watch.util test, covering Exception handling

299067f

Add tests for pytest_watch.helpers samepath & deque_all

e6b26b8

Added test for named space files on samepath

5f2cf64

gitignore coverage.xml

03c243b

Added pytest-runner to setup.py and 'setup.py test' alias for 'pytest'

2ef5b86

Skip tests on win32 system for checking real path for symlinks. Win32…

42a64da

… doesnt support symlinks

wrapper for build win32 command line path

c944899

subprocess.call working for Windows paths

e0beb47

Create appveyor.yml

61083c9

Update appveyor.yml

1913c66

Added classifier Famework :: Pytest for better pypi index classification

bb39e90

apast force-pushed the 14-add-tests branch from da482e8 to bb39e90 Compare May 21, 2018 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#14 Add tests. pytest_watch.command and pytest_watch.watcher #79

#14 Add tests. pytest_watch.command and pytest_watch.watcher #79

apast commented Jan 3, 2018 •

edited

joeyespo Jan 4, 2018

joeyespo Jan 4, 2018 •

edited

joeyespo Jan 4, 2018

joeyespo Jan 4, 2018

joeyespo Jan 4, 2018

joeyespo Jan 4, 2018

joeyespo Jan 4, 2018

apast Jan 4, 2018

joeyespo Jan 4, 2018 •

edited

joeyespo Jan 4, 2018 •

edited

apast Jan 4, 2018

joeyespo Jan 5, 2018 •

edited

apast Jan 6, 2018

joeyespo Jan 4, 2018

joeyespo Jan 4, 2018

apast Jan 4, 2018 •

edited

joeyespo left a comment

joeyespo commented Jan 4, 2018 •

edited

joeyespo commented Jan 4, 2018

apast commented Jan 4, 2018

apast commented Jan 4, 2018

apast commented Jan 4, 2018

		from pytest_watch.__main__ import run_cli


		class TestRunCLI(unittest.TestCase):

#14 Add tests. pytest_watch.command and pytest_watch.watcher #79

Are you sure you want to change the base?

#14 Add tests. pytest_watch.command and pytest_watch.watcher #79

Conversation

apast commented Jan 3, 2018 • edited

Choose a reason for hiding this comment

joeyespo Jan 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyespo Jan 4, 2018 • edited

Choose a reason for hiding this comment

joeyespo Jan 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyespo Jan 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apast Jan 4, 2018 • edited

Choose a reason for hiding this comment

joeyespo left a comment

Choose a reason for hiding this comment

joeyespo commented Jan 4, 2018 • edited

joeyespo commented Jan 4, 2018

apast commented Jan 4, 2018

apast commented Jan 4, 2018

apast commented Jan 4, 2018

apast commented Jan 3, 2018 •

edited

joeyespo Jan 4, 2018 •

edited

joeyespo Jan 4, 2018 •

edited

joeyespo Jan 4, 2018 •

edited

joeyespo Jan 5, 2018 •

edited

apast Jan 4, 2018 •

edited

joeyespo commented Jan 4, 2018 •

edited