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

use Watchdog in auto command #101

Closed
wants to merge 16 commits into from
Closed

use Watchdog in auto command #101

wants to merge 16 commits into from

Conversation

schettino72
Copy link
Member

Problems

all

  • detect event file move (also check behavior for create and delete)

windows

mac

  • check it really works (command line) on python?
  • test_cmd_auto.TestAuto.test_run_wait (all py versions) - error not terminated
  • test_filewatch testLoop and testExit HANGS (test currently skipped)

linux

  • test_filewatch testExit (py2.7) HANGS (test currently skipped)

@schettino72
Copy link
Member Author

Mac guys: @philipbl @barleyj . Can you please help me test this branch? MAC support for command auto.

@Kwpolska
Copy link
Contributor

Kwpolska commented Sep 3, 2015

Any Windows testers? I could volunteer if given a test dodo.py file.

@schettino72
Copy link
Member Author

Windows guys: @joschkazj @gstorer.

I am working on supporting the auto command on Windows. Before I look into more pickle issues I would like to investigate why the test below is failing. Can you help me?

https://ci.appveyor.com/project/schettino72/doit/build/1.0.74/job/7jlkh5xuv1g282ni#L392

@schettino72
Copy link
Member Author

@Kwpolska better start looking at the test failure I mentioned above. At the moment I guess the code wont run at all on Windows.

sample dodo.py

import time
import random

DOIT_CONFIG = {'verbosity': 2}


def luck():
    print('start...')
    time.sleep(5)
    result = random.random() > 0.5
    print('finish.')
    #return True
    return result

def task_simple():
    return {
        'actions': ['echo hi', luck],
        'file_dep': ['foo.txt'],
    }

@Kwpolska
Copy link
Contributor

Kwpolska commented Sep 3, 2015

@schettino72 this could just be a race condition, maybe try to throw in a time.sleep()?

@schettino72
Copy link
Member Author

I dont think it is a race condition because the first assert passed. Seems the file was added twice into the list of events...

I am not really willing to debug code using appveyor so I am asking for help. or windows will still lag behind.

@philipbl
Copy link

philipbl commented Sep 3, 2015

@schettino72 On my Mac setup with Python 3, test_filewatch.py hangs on testLoop. You have to ctrl-c to exit the tests. Also, on test_cmd_auto.py, test_run_wait fails saying that the process was not terminated.

I don't have time to look into this right now, but I wanted to let you know the results.

@philipbl philipbl mentioned this pull request Sep 4, 2015
@philipbl philipbl mentioned this pull request Sep 4, 2015
@philipbl
Copy link

philipbl commented Sep 4, 2015

Using doit from the command line, auto is working great on OS X!

As a point of reference, here is the dodo.py file I used for testing:

def task_test():
    return {'actions': ["cat file1 file2 > out"],
            'file_dep': ["file1", "file2"],
            'targets': ["out"]
            }

Running doit auto and changing file1 or file2 causes the task to be run again.

@schettino72
Copy link
Member Author

@philipbl thanks. It got stuck the same way :( and have more failures. Can you please take a look into it?

@joschkazj
Copy link

@schettino72's sample works fine for me on the Windows command-line. I had to quit doit via Ctrl+Break though, Ctrl+C had no effect for me.

EDIT: This is on Python 2.7.9

Failing tests in test_cmd_auto.py:

  • TestDepChanged.test_changed (same result as in AppVeyor)
  • TestAuto.test_run_wait: PicklingError: Can't pickle <function ok at 0x0000000003C0C518>: it's not found as tests.test_cmd_auto.ok

To me the pickling issue is to be expected as the function ok() is not part of the test class instance but only defined at runtime of the method. I had similar issue when testing the multiprocessing fix on Windows.

@schettino72
Copy link
Member Author

Thanks @joschkazj . It is a good news the fact the problems are specific to tests.

About Ctrl+Break vs Ctrl+C I am completely clueless. I didnt even know about the existence of a Ctrl+Break. How other command line tools behave on Windows?

What about tests.test_filewatch.TestFileWatcher.testLoop() ? It also fails in your machine?
It makes sense to start fixing this one because a problem here might affect the tests command auto too.

It would be great if you can investigate further the issues and propose a patch fixing them 😬

@joschkazj
Copy link

What about tests.test_filewatch.TestFileWatcher.testLoop() ? It also fails in your machine?

No, it seems to work (the skipped test is due to Python 2.x):

tests\test_filewatch.py ...s

About Ctrl+Break vs Ctrl+C I am completely clueless. I didnt even know about the existence of a Ctrl+Break. How other command line tools behave on Windows?

Ctrl+C can be caught by the application (e.g. IPython kernels react to it and exit gracefully), while Ctrl+Break is sent directly to the Windows OS and kills the application (official documentation).

@schettino72
Copy link
Member Author

The test being skipped on python2.7 is testExit, I didnt manage to get this test working on Linux.
But the functionality is ok. This test should check that Ctrl-C works...

So the testLoop issue is only on AppVeyor... 😞

@schettino72
Copy link
Member Author

@philipbl with which version of python you tested the command line OS X ?

@barleyj
Copy link

barleyj commented Sep 8, 2015

OS X works.

One issue that I've noticed when using doit with TDD, a failing test causes doit to continuously run until you fix the test. I'm not sure if this is intended or not. It doesn't appear to be related to this change.

@schettino72
Copy link
Member Author

@barleyj

thanks for the feedback

One issue that I've noticed when using doit with TDD, a failing test causes doit to continuously run until you fix the test. I'm not sure if this is intended or not.

This is not intended and should be already be fixed in this branch. Do you have an example?

There are too many issues with tests. I will not merge this branch in the current situation...

Another thing that makes me think twice is the state of watchdog,
the project is not really actively maintained and they themselves do not run their test suite on OS X nor Windows. I am afraid merging this will be a step backwards.

@schettino72
Copy link
Member Author

looks like watchdog project is dead. not a single commit last year...

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.

None yet

5 participants