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

win32 fixes to avoid unix only apis in tricks/__init__.py #810

Merged
merged 7 commits into from Jun 29, 2021

Conversation

replabrobin
Copy link
Contributor

OK I have toxed this in linux, but I will need time to check the changes in windows 10

@BoboTiG BoboTiG linked an issue Jun 29, 2021 that may be closed by this pull request
@BoboTiG BoboTiG closed this Jun 29, 2021
@BoboTiG BoboTiG reopened this Jun 29, 2021
@BoboTiG
Copy link
Collaborator

BoboTiG commented Jun 29, 2021

I just saw that watchmedo tests are near to zero. Do you think you could add simple test(s) to cover your changes?

@replabrobin
Copy link
Contributor Author

replabrobin commented Jun 29, 2021

Hi I'm not really certain how one should test this; I used the following code



def make_dummy_script(tmpdir, n=10):
	script = os.path.join(tmpdir, 'auto-test-%d.py' % n)
	with open(script, 'w') as f:
		f.write('import time\nfor i in range(%d):\n\tprint("+++++ %%d" %% i)\n\ttime.sleep(1)\n' % n)
	return script


def test_kill_auto_restart(tmpdir):
	from watchdog.tricks import AutoRestartTrick
	import sys
	import time
	script = make_dummy_script(tmpdir)
	try:
		a = AutoRestartTrick([sys.executable, script])
		a.start()
		time.sleep(5)
		a.stop()
	finally:
		os.remove(script)

but as a person who doesn't normally use Tox I don't know to test that the kill actually stops the task in the middle. There was no error and the test said it passed, but I would like to check that we didn't get to "+++++ 9" in the output.

EDIT: apparently I need to start messing with pytest capsys plugin. I will take a few moments to figure out what to do.

Copy link
Collaborator

@BoboTiG BoboTiG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the test, really appreciated!

tests/test_watchmedo.py Outdated Show resolved Hide resolved
tests/test_watchmedo.py Outdated Show resolved Hide resolved
@replabrobin
Copy link
Contributor Author

Not sure why these errors are so random. I tried in a forked rep and got different failures :(

@BoboTiG
Copy link
Collaborator

BoboTiG commented Jun 29, 2021

Random failures are OK as soon as the new test passes on Windows.

@replabrobin
Copy link
Contributor Author

I'm not windows savvy any more. I will have to try and run these on a windows 10 laptop and see if we never get the KeyboardInterrupt appearing in the subprocess output.

@replabrobin
Copy link
Contributor Author

OK tried various tricks to get the stderr from the subprocess and in windows it doesn't work. So the easiest is to just drop the requirement that the right exception is seen.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Jun 29, 2021

That's good for me, thanks @replabrobin 🍾

@BoboTiG BoboTiG merged commit 44ced23 into gorakhargosh:master Jun 29, 2021
@replabrobin
Copy link
Contributor Author

Thanks BoboTiG; I'm never sure about the git philosophy here. Should I leave the branch in place in my repo or delete it or delete the whole repo? What's the 'right/nice' way?

@BoboTiG
Copy link
Collaborator

BoboTiG commented Jun 30, 2021

As you have another PR, it is a good practice to only delete the branch since it was merged.
You could delete the whole repository when all your PRs will be either merged either closed (or you can keep the repository alive, it is up to you).

@replabrobin replabrobin deleted the win32-watchmedo-fixes branch June 30, 2021 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

windows tracebacks caused by use of os.setsid & os.killpg
2 participants