[WIP] Watch feature

[vck3000]

Closes #246.

Hey @CleanCut! I've had some time to give this a go, but I need some advice as I'm running into some issues.

The current problem is that on file save, the tests successfully re-run, however it is re-run on the old code. It appears that it does not load in the newer, just saved code.

The implementation used to watch file changes is the watchdog library that is cross-platform compatible, but I understand it brings in a new dependency which you may not want. I also wasn't sure how to add it to the requirements.txt file, as the usual pip3 freeze > requirements.txt ended up changing pretty much the whole file...!

I wanted to re-run the tests inside the GreenEventHandler I wrote, however I ran into issues with signal not being able to run outside of the main thread. Therefore I went with a simple global semaphore implementation as I'm not sure how else to deal with this!

I'm still fairly new to contributing to larger projects, but I'm happy to put in the time and to learn if you can point me in the right direction!

[CleanCut]

I'm still fairly new to contributing to larger projects, but I'm happy to put in the time and to learn if you can point me in the right direction!

Welcome! I'm happy to steer you in the right direction. 😄 I enabled CI checks for you, so you can see what things work (or don't) on the various environments.

The current problem is that on file save, the tests successfully re-run, however it is re-run on the old code. It appears that it does not load in the newer, just saved code.

TL;DR I don't think a reloading strategy is best architectural approach to the problem. My suggestion would be to look into wrapping green entirely, instead.

The design of unittest (and of Python in general) assumes that if you want to reload something, you'll exit Python and start it again from scratch. There is limited support in the language for reloading modules, but I wouldn't feel comfortable relying on it, as whether it worked or not would be up to the behavior of the code you are reloading. Quite frankly, most Python code never even considers what would happen if it were reloaded, so I expect there would be never-ending project-specific problems. Even if reloading worked reliably in all cases, you would have to monkey-patch support for reloading into unittest, which is invasive enough that it would probably end up being easier to fork and alter the entire built-in unittest module, which is something I definitely don't want to do.

Here is the approach I think might work: Write (or use) a script that does the watching and launches green itself as a subprocess of that script. Once you've got that working, have green's argument handle the -w argument and re-exec itself as the wrapper script. Here's what it would look like as a sequence of events:

1. User launches green -w -vv sometarget
2. Green strips off -w and uses one of the os.exec* functions to replace itself with somewrapper -vv sometarget
3. "somewrapper" then watches sometarget and whenever something changes...
   1. it launches green -vv sometarget in a subprocess.

Honestly, somewrapper doesn't even have to be something you write. It could just be watchmedo or some other utility that is cross-platform and straightforward to instruct people to install. In that case, it would be more like:

1. User launches green -w -vv sometarget 1a. If watchdog is not installed, the user is prompted with instructions to install it.
2. Green strips off -w and uses one of the os.exec* functions to replace itself with watchmedo shell-command --patterns="*.py" --recursive --command='green -vv sometarget' sometarget

The implementation used to watch file changes is the watchdog library that is cross-platform compatible, but I understand it brings in a new dependency which you may not want. I also wasn't sure how to add it to the requirements.txt file, as the usual pip3 freeze > requirements.txt ended up changing pretty much the whole file...!

I would prefer not to add a dependency that few people will actually use. How about we check to see if it's installed when someone does the -w option, and if it's not installed we emit a message telling them to install it?

To answer you actual question: The way you would add watchdog as a dependency is to manually add a line watchdog to requirements.txt--though I don't want to do that. You should add it to requirements-optional.txt, though, so it is installed for CI. pip freeze outputs all the packages installed in your environment, which is definitely not what we want. 😜

I wanted to re-run the tests inside the GreenEventHandler I wrote, however I ran into issues with signal not being able to run outside of the main thread. Therefore I went with a simple global semaphore implementation as I'm not sure how else to deal with this!

I'm not really sure what you mean, here. You don't have any signal in your current code, and Green doesn't use threads (it uses subprocesses). Perhaps if you took the architectural approach I outlined (write some sort of wrapper), then you could give your signal approach another try?

[vck3000]

Wow, thank you for such a detailed reply! Really appreciate all the pointers! I'll have another crack at it and will keep you updated :).

[vck3000]

Hey @CleanCut! Sorry I took so long to get back to this. Just started at a new place and have been a bit under the crunch!

I've given it a go and wanted to update you on how I went :).

I've followed your suggestion of using watchmedo to wrap green. It works quite well, especially once the --drop argument was added which prevents watchmedo from running green multiple times due to the multiple events it fires when a file is modified!

I did have a couple more questions coming out of this though if you didn't mind...!

1. I found that I couldn't get os.execvp() to output to my terminal screen at all. I did find some information about os.exec*() not inheriting file descriptors by default anymore since Python 3.4 (https://docs.python.org/3/library/os.html#inheritance-of-file-descriptors) , but I couldn't work out how to get access to the sys.stdout's raw file descriptor to use os.set_inheritable() on it... (not even sure if this is the problem...)

To get around this, I found that calling os.system() worked well. I see in the docs that it creates a sub-shell, and was wondering if that was adequate for our purposes here :).

2. watchmedo shell-command doesn't execute an initial run on first launch, meaning that it only runs the test after a file is modified, and an event fired. I found that watchmedo supports the auto-restart command, which looks like it has the desired behaviour for this, but it starts emitting some un-suppressible messages :(.

They look like this: https://user-images.githubusercontent.com/42633156/126863175-d64fc424-4bc8-47b7-a5f5-a550d71ef8cc.png

I've left in the code there if you wanted to take a look at the auto-restart part.

To get around that, I've simply called green once before the watchmedo shell-command call using another os.system() call. Does this sound ok? :)

3. I also wanted to include some "console clearing on each re-run" functionality too. For now, it's hard coded, using the shell clear command, which isn't going to work on Windows. I found some ways of doing this like this, but this relies on the Python program's runtime (i.e. green) to clear the screen. I'm thinking this means that we would have to add a new private flag to green's CLI to signal to the app to clear the screen when we're running in watch-mode with clear screen active, which doesn't sound like the best of ideas haha.

Did you have any ideas on this? I guess console screen clearing isn't the most necessary of features, but I think it'd be nice to have :).

4. Currently I'm removing the -w watch flag by hard coding it, before passing it down to watchmedo. I noticed that the CLI library supports combining short flags together, e.g. green -vvvr will run in verbose mode AND run coverage. This means that a user could possibly give green -vvvwr and want it to also run in watch mode. (I'll also have to consider the long flag --watch later)

The current idea I have to deal with this is to look for an argument that has a single dash (to represent a short flag), and then removing all w entries found, but I'm not sure if this is the best way to go about it. Any tips for this? :D.

5. Does the missing watchdog library error message I came up with sound ok?

6. This is probably the one I'm going to have the most trouble with... Do you have any tips on how I could write some end to end tests for this kind of behaviour? I'm not sure how I can go about automating a test for this because it exists outside the normal program's running...

7. A bit on the side, but I found I had to install argh to use watchmedo. On watchdog's README, they state that argh is a dependency for watchmedo, and isn't installed when one does pip3 install watchdog. I'm not sure if it's the best idea for us to also include a check for argh, and the other dependency they state for watchmedo, since it might be coupling us to their dependency list too hard.

All these questions aside though, it's looking really cool so far! Demo gif.

I understand that this watch feature may be going a bit outside of what green's main goals are, so if you find this to be too much, I would be totally happy to drop this and try to help you on another "good first issue" :). I'm learning a lot of good stuff on the way already! Thank you!

[vck3000]

On some after thought, I think it might be good to simply write a complete separate Python package that wraps green and watchmedo. That way it'd remove the need for a watch flag (and it's tricky removal from the args list), it'd just need to pass down all the args to green, it can avoid having to get the user to install watchdog and argh separately, and it could handle screen clearing.

I think that might be a fun venture for me to play with too haha. Happy to hear your thoughts :D.

[CleanCut]

Sorry it took me a whole week to get to this. Life has been crazy!

---

First off, your demo looks cool! 😎

On some after thought, I think it might be good to simply write a complete separate Python package that wraps green and watchmedo. That way it'd remove the need for a watch flag (and it's tricky removal from the args list), it'd just need to pass down all the args to green, it can avoid having to get the user to install watchdog and argh separately, and it could handle screen clearing.

I think that might be a fun venture for me to play with too haha. Happy to hear your thoughts :D.

That's a great idea! I think you should go ahead and try making your own project. That gives you a nice boundary for functionality and full ownership of how you want to do things. If it doesn't work out, you can always come back and continue this PR.

I'll respond to your other questions too, though maybe they won't be as relevant given this ☝🏻

1. I found that I couldn't get os.execvp() to output to my terminal screen at all. I did find some information about os.exec*() not inheriting file descriptors by default anymore since Python 3.4 (https://docs.python.org/3/library/os.html#inheritance-of-file-descriptors) , but I couldn't work out how to get access to the sys.stdout's raw file descriptor to use os.set_inheritable() on it... (not even sure if this is the problem...)

🤔 Hmm. That's really odd that you weren't getting output. My understanding of the documentation you linked to is that it is only talking about non-inheritable file-descriptors that you create through your own open('somefile') calls. It specifically says that stdout, stderr, (etc.) are still inheritable. You really should be able to use this (or a closely-related) function and still have stdout/stderr hooked up.

To get around this, I found that calling os.system() worked well. I see in the docs that it creates a sub-shell, and was wondering if that was adequate for our purposes here :).

Unfortunately, no. One of the reasons the os.exec* functions are so nice is that they don't run the arguments through shell interpretation. Joining all the arguments into a string and passing it to os.system() again causes the arguments to be re-evaluated in a shell, which can cause all sorts of problems with re-interpretation of the arguments. For example

# This command
green --config 'money$bags.cfg' -w "space in directory/module.py" 

# becomes this argv the first time (this is fine)
["green", "--config", "money$bags.cfg", "space in directory/module.py"]

# and when you do " ".join(...) it becomes (...see the problem yet?)
"green --config money$bags.cfg space in directory/module.py"

# which then becomes this argv after being re-interpreted by os.system()'s shell,
# assuming that $bags is undefined as a shell variable.  (Completely mangled! 😭)
["green", "--config", "money.cfg", "space", "in", "directory/module.py"]

Preventing a shell from mangling an already-interpreted command-line is hard, and not very fun. Better to avoid the shell altogether.

I also wanted to include some "console clearing on each re-run" functionality too. [snip] Did you have any ideas on this?

This one I can help you with directly! This should work on posix and windows:

import colorama
colorama.init()
print(colorama.ansi.clear_screen(), colorama.Cursor.POS(0, 0), sep="", end="")

Currently I'm removing the -w watch flag by hard coding it, before passing it down to watchmedo. [snip] Any tips for this? :D.

As you noted, you can skip this problem entirely by making your own wrapper project. 😉 If you were to keep going on this PR, I would suggest trying a completely different approach -- using the watchdog API inside of green, so you don't need to reinterpret arguments at all.

Does the missing watchdog library error message I came up with sound ok?

Sure, sounds reasonable.

This is probably the one I'm going to have the most trouble with... Do you have any tips on how I could write some end to end tests for this kind of behaviour? I'm not sure how I can go about automating a test for this because it exists outside the normal program's running...

Yes! You'll want to write an integration test instead of a unit test. The main difference is your approach to implementing the test. Instead of importing your own code and testing it directly, use subprocess.Popen to run your project in a subprocess. Then you can use that Popen object to repeatedly grab any output after poking at various files.

I would be totally happy to drop this and try to help you on another "good first issue" :). I'm learning a lot of good stuff on the way already! Thank you!

No pressure from me, but I'm always happy if folks want to try to improve Green. 😄

[vck3000]

Sorry it took me a whole week to get to this. Life has been crazy!

No worries at all! Thank you for the detailed feedback again! I really appreciate your time!

That's a great idea! I think you should go ahead and try making your own project. That gives you a nice boundary for functionality and full ownership of how you want to do things. If it doesn't work out, you can always come back and continue this PR.

Sounds good! I'll give it a go sometime :).

You really should be able to use this (or a closely-related) function and still have stdout/stderr hooked up.

Hmmm, I'll have to try this again.

One of the reasons the os.exec* functions are so nice is that they don't run the arguments through shell interpretation. [clip] which then becomes this argv after being re-interpreted by os.system()'s shell, [clip] (Completely mangled! 😭)

Ahhh I see. Very interesting! That must be why I've seen so many functions that spawn processes accepting a list of args, rather than a single string of args directly...! I can feel the puzzle pieces falling into place! Thank you :).

I would suggest trying a completely different approach -- using the watchdog API inside of green, so you don't need to reinterpret arguments at all.

Could you explain a bit more on how this would work? This kind of sounds like how I attempted it in my first commit. In it, I set up watchdog to listen to a directory, and then re-loaded the tests, however your advice was that re-loading tests would be difficult due to the underlying unittest library.

In that case, would the approach here be:

1. Detect the watch flag.
2. Start up watchdog api to listen for changes in the parent Green process.
3. Spawn a new Green sub-process to run the tests.
4. On watchdog file update event, go to 3.

If so, do you know how I would be able to pass the currently parsed args in the parent Green instance (that is running watchdog) to the child Green sub-processes that run the tests, without extracting the -w flag out manually from the sys.argv strings? Is there a way to pass a whole args object directly in?

Instead of importing your own code and testing it directly, use subprocess.Popen to run your project in a subprocess. Then you can use that Popen object to repeatedly grab any output after poking at various files.

Ah I see! That sounds very interesting and I would love to give that a go :).

Although I think I'll give the wrapper project a go (to learn more about the Python ecosystem and to have some fun :P), I think it may still be nice to have it built into Green too and I'd love to be able to contribute something, given the amount of help you've given me :).

Edit: I just found out about pytest, and it looks like someone else had the same idea of wrapping it to re-run on file changes!

[CleanCut]

Could you explain a bit more on how this would work? This kind of sounds like how I attempted it in my first commit. In it, I set up watchdog to listen to a directory, and then re-loaded the tests, however your advice was that re-loading tests would be difficult due to the underlying unittest library.

In that case, would the approach here be:

1. Detect the watch flag.
2. Start up watchdog api to listen for changes in the parent Green process.
3. Spawn a new Green sub-process to run the tests.
4. On watchdog file update event, go to 3.

Yes, exactly!

Although I think I'll give the wrapper project a go (to learn more about the Python ecosystem and to have some fun :P), I think it may still be nice to have it built into Green too and I'd love to be able to contribute something, given the amount of help you've given me :).

Sounds like a good plan. 👍🏻

Edit: I just found out about pytest, and it looks like someone else had the same idea of wrapping it to re-run on file changes!

Always nice to have prior art out there to see how someone else did something similar!

[CleanCut]

I'm going to go ahead and close this, since it seems like there might not be any further activity. We can reopen it in the future, if needed.