Skip to content
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

Multiple directories support, norecursedirs, passing args to pytest #11

Merged
merged 20 commits into from
Jun 13, 2015

Conversation

aldanor
Copy link
Contributor

@aldanor aldanor commented May 27, 2015

I believe this PR closes all outstanding issues (#5, #6, #7 and #8). If merged in, this probably calls for a version bump.

The README will probably need to be updated as well..

Options added / things modified:

  • All positional arguments after -- are now passed as is to py.test executable.
  • Added support for multiple watched folders (passed as positional arguments).
  • Allow to ignore sub folders (currently via an exact match only at a first level of each watched folder, but this could be improved quite easily to support arbitrary patterns). Note that this is done at the observer level and not in the event handler because if you have tens of thousands of files in a subfolder like .tox for example, the handler would get completely swamped by that (so a simple pattern matching inside on_any_event is not going to cut it).
  • py.test subprocess no longer changes directory to a watched folder -- this no longer makes any sense given that any args can be passed to pytest and you can watch multiple folders at once.
  • py.test subprocess no longer runs with shell=True, to handle quoted arguments properly.
  • Catch KeyboardInterrupt even if it was caught after the first test run and exit gracefully.
  • Instead of catching all file events, only react to FileModifiedEvent and FileCreatedEvent. This fixed the problem where duplicate watchdog events would be fired upon a single file modification if using remote NFS mounts on Linux.
  • Added -p alias to --poll.
  • The exact py.test command is now printed to stdout upon the first test run.
  • Added event spooling with 200ms cooldown to work around atomic saves in different editors.
  • Added --verbose / --quiet options.
  • Added --no-spool option to disable spooling.
  • --ext option will prepend extensions with periods if needed.

An example using the new functionality:

ptw -v -p --no-spool --ignore=.tox,.git folder1 folder2 -- tests/*.py -svv

- This fixes the problem with catching duplicate watchdog events
  related to a single file modification if using remote NFS mounts.
…ge cwd.

- Positional arguments are now passed directly to py.test executable
  and can be separated from options via the optional "--"
- py.test subprocess no longer changes the current directory
- Directories are now specified via --watch option (comma-separated),
  default is the current directory.
- Added "-p" alias for "--poll"
This fixes the edge case when py.test arguments contained quoted strings
with spaces, e.g.:

    ptw -- -m 'not slow'
This fixes the issue with some editors that do atomic saves via a
temporary file and a subsequent move.
@joeyespo
Copy link
Owner

This all looks great! Thank you for putting this all together, @aldanor!

Also, really clear PR message. I really appreciate the writeup. It set good expectations. And the example usage was helpful to envision these changes.

I'm all ready to pull it in. Just address those few comments first. For convenience:

  • Use [directories] instead of --watch to keep it simple and backwards compatible?
  • Can you think of any cases where shell=True is an advantage here?
  • If not, go ahead and remove the redundant shell=False
  • Are file renames picked up by the current events?
  • --ignore instead of --nonrecursedirs?

I can also bump the version and update the README after.

@joeyespo
Copy link
Owner

Nevermind about --norecursedirs. I just remembered that came from py.test.

Do you think we should also pass that argument to py.test? Otherwise, py.test might still run tests even though we're not listening for changes in those deeper directories.

@aldanor
Copy link
Contributor Author

aldanor commented May 28, 2015

Renaming --norecursedirs to --ignore seems reasonable. Passing it to py.test automatically is probably not, as you may want to listen to a restricted set of subfolders while still running the full test suite, I think -- these are two different notions. You could do it the other way around though and use norecursedirs from pytest config file (which is most always present anyway) as a default value for --ignore.

@aldanor
Copy link
Contributor Author

aldanor commented May 28, 2015

@joeyespo The --watch vs [directory] ([directories]) question is still open -- if you can suggest an unambiguous docopt signature for this I'd be happy to fix it.

…d by --)

Also, make --ext a bit smarter by prepending the '.' automatically if need be.
@aldanor
Copy link
Contributor Author

aldanor commented Jun 10, 2015

@joeyespo CLI args fixed, you can try testing it now. What's remaining is event spooling, will hack on it now.

@aldanor
Copy link
Contributor Author

aldanor commented Jun 10, 2015

@joeyespo Event spooling done:

Changes detected:
    deleted:  pytest_watch/watcher.py
    moved:    pytest_watch/.sublc49.tmp -> pytest_watch/watcher.py

Rerunning pytest command: py.test --import .

I guess I'll have to add -v / -q options for verbose/quiet output, and also an option to opt out of event spooling (should it be enabled by default? the default cooldown is 200ms).

@aldanor aldanor mentioned this pull request Jun 10, 2015
@blueyed
Copy link
Collaborator

blueyed commented Jun 10, 2015

@aldanor
I've noticed that the text of the modified filenames and the command in "Rerunning pytest command" is not visible in my terminal, but can be copy'n'pasted.

@blueyed
Copy link
Collaborator

blueyed commented Jun 10, 2015

Instead of using highlight = lambda arg: Fore.LIGHTWHITE_EX + arg + Fore.CYAN it might be better to just make it bold?

diff --git i/pytest_watch/watcher.py w/pytest_watch/watcher.py
index abe97f7..ebdaa2f 100644
--- i/pytest_watch/watcher.py
+++ w/pytest_watch/watcher.py
@@ -6,7 +6,7 @@

 from .spooler import EventSpooler

-from colorama import Fore
+from colorama import Fore, Style
 from watchdog.observers import Observer
 from watchdog.observers.polling import PollingObserver
 from watchdog.events import (FileSystemEventHandler, FileModifiedEvent,
@@ -60,7 +60,7 @@ def run(self, summary=None):
         if self.auto_clear:
             subprocess.call(CLEAR_COMMAND)
         command = ' '.join(['py.test'] + self.args)
-        highlight = lambda arg: Fore.LIGHTWHITE_EX + arg + Fore.CYAN
+        highlight = lambda arg: Style.BRIGHT + arg + Style.NORMAL
         msg = 'Running pytest command: {}'.format(highlight(command))
         if summary:
             msg = 'Changes detected in files:\n{}\n\nRerunning pytest command: {}'.format(

@tarpas
Copy link

tarpas commented Jun 10, 2015

On Mac OS the spooling seems to cause problems.
tibor$ ptw

Running pytest command: py.test
=============================================== test session starts ================================================
platform darwin -- Python 2.7.8 -- py-1.4.28 -- pytest-2.7.1
rootdir: /Users/tibor/tmonworkspace/testmon/exampleproject, inifile: pytest.ini
plugins: testmon, cache, html
collected 13 items 
  tests/test_a.py sx..  tests/test_ab.py .  tests/test_b.py ..  tests/single/test_s.py ...  tests/single/test_t.py ..  tests_2/test_e.py s
================================= 10 passed, 2 skipped, 1 xfailed in 0.08 seconds ==================================
Process Timer-1:
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/multiprocessing/process.py", line 258, in _bootstrap
    self.run()
  File "/Users/tibor/tmonworkspace/pytest-watch/pytest_watch/spooler.py", line 21, in run
    self.function(*self.args, **self.kwargs)
  File "/Users/tibor/tmonworkspace/pytest-watch/pytest_watch/spooler.py", line 39, in process
    self.callback([self.outbox.get() for _ in range(self.outbox.qsize())])
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/multiprocessing/queues.py", line 143, in qsize
    return self._maxsize - self._sem._semlock._get_value()
NotImplementedError

@joeyespo
Copy link
Owner

@aldanor There's a lot of divergent subtle cases being handled in this PR. If it's getting too unwieldy, perhaps open a few different ones to address specific issues. I'm very ready to pull in most things in here, but until it's completely ready, the whole thing is held up.

@aldanor
Copy link
Contributor Author

aldanor commented Jun 10, 2015

I got to my mac, will run the whole thing on it tonight.

I'm actually thinking this whole thing needs a test suite...

@aldanor
Copy link
Contributor Author

aldanor commented Jun 10, 2015

@joeyespo Yep I know.... it will be real hard for me to start going backwards and split it up because I have to use it on daily basis, which means I'll have to keep track of more than three versions now -- remote, local and multiple PR branches... :(

I'd rather make this whole thing work. (the norecursedirs changes I can roll back -- that's the only truly questionable part which will need further effort and thought).

@aldanor
Copy link
Contributor Author

aldanor commented Jun 10, 2015

@tarpas Fixed. OSX weirdness...

@aldanor
Copy link
Contributor Author

aldanor commented Jun 10, 2015

@blueyed Fixed

@aldanor
Copy link
Contributor Author

aldanor commented Jun 10, 2015

Anything else?

@tarpas
Copy link

tarpas commented Jun 11, 2015

Works without problems for me also with pytest testmon. Thanks for the
work! @joeyespo it would be great if you merge it.

On Wed, Jun 10, 2015 at 11:02 PM, Joe Esposito notifications@github.com
wrote:

@aldanor https://github.com/aldanor There's a lot of divergent subtle
cases being handled in this PR. If it's getting too unwieldy, perhaps open
a few different ones to address specific issues. I'm very ready to pull in
most things in here, but until it's completely ready, the whole thing is
held up.


Reply to this email directly or view it on GitHub
#11 (comment).

@joeyespo
Copy link
Owner

Yep I know.... it will be real hard for me to start going backwards and split it up because I have to use it on daily basis, which means I'll have to keep track of more than three versions now -- remote, local and multiple PR branches

Yeah, it'd be kind of tough now. Oh well.

Pulling this is now so we can move forward. We can fix up --ignore / --norecursedirs stuff after. Lots of good work in this PR.

Many thanks, @aldanor 😃

@joeyespo
Copy link
Owner

@blueyed Did 0a24376 do what you need? If not, could you open a new issue?

joeyespo added a commit that referenced this pull request Jun 13, 2015
Multiple directories support, norecursedirs, passing args to pytest
@joeyespo joeyespo merged commit de5d34d into joeyespo:master Jun 13, 2015
@joeyespo
Copy link
Owner

Thanks again, @aldanor for your fantastic work here. And for your patience :-)

@aldanor
Copy link
Contributor Author

aldanor commented Jun 13, 2015

@joeyespo Np :) I think all the outstanding issues have been covered, everyone's happy? (--ignore is a separate issue but it actually does somewhat work already)

Btw, maybe it would make sense to move this thing over to http://github.com/pytest-dev? (seeing as pytest itself will move there soon)

@aldanor
Copy link
Contributor Author

aldanor commented Jun 13, 2015

This one was one hell of a PR, more Python LOC than the original code :)

@joeyespo
Copy link
Owner

Btw, maybe it would make sense to move this thing over to http://github.com/pytest-dev?

I'm up for it if they are.

This one was one hell of a PR, more Python LOC than the original code :)

😃

@aldanor
Copy link
Contributor Author

aldanor commented Jun 13, 2015

What exactly is Linux-only? I'm mostly on OSX myself (occasionally on Linux)

@joeyespo
Copy link
Owner

@aldanor Oops, meant Unix, not Linux. os.path.samefile is Unix-only.

@blueyed
Copy link
Collaborator

blueyed commented Jun 14, 2015

@joeyespo

@blueyed Did 0a24376 do what you need? If not, could you open a new issue?

Yes, thanks!

@joeyespo
Copy link
Owner

FYI released these changes today. Be sure to pip install --upgrade pytest-watch 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants