Run unit tests from executable Python modules

Review Request #5325 — Created Jan. 26, 2014 and submitted

tomiaijo
Djblets
master
djblets

Run unit tests from executable Python modules

When developing on a Linux VM inside a Windows host, all Python modules have the executable flag on. Nose has work around for this when run on Windows (https://github.com/nose-devs/nose/blob/master/nose/config.py#L26). In this case, the Python intrepeter run inside the VM causing the work-around to fail. This review request adds check for executable file permission and forces Nose to run tests.

Unit tests pass.

Description From Last Updated

Make sure the import is listed alphabetically.

chipx86chipx86

Need to keep two blank lines between these.

chipx86chipx86

This is only true if running on Windows. A better comment would be something like: "If the test files are ...

chipx86chipx86

Blank line before the if statement. Instead of os.getcwd(), it's best to be explicit about file paths. You also don't ...

chipx86chipx86

We use parens for multi-line if statements. This should be more like: if (os.path.exists(...) and (os.stat(...) & ...)): Note also ...

chipx86chipx86

You have some trailing whitespace on these lines (where it highlights in red in the diff viewer).

daviddavid

Can you indent 'settings.py' to line up with os.path.dirname?

daviddavid
chipx86
  1. Congrats on your first change!

    So as you'll see from this, we (certainly I) can be nit-picky, especially in your first few changes, in order to hammer in Pythonisms, style guideline stuff, and best practices. That's pretty common in the industry, so it's important not to worry about it :)

  2. tests/runtests.py (Diff revision 1)
     
     

    Make sure the import is listed alphabetically.

  3. tests/runtests.py (Diff revision 1)
     
     
     
     
     

    Need to keep two blank lines between these.

  4. tests/runtests.py (Diff revision 1)
     
     

    This is only true if running on Windows. A better comment would be something like: "If the test files are executable on the file system, nose will need the --exe argument to run them."

  5. tests/runtests.py (Diff revision 1)
     
     
     

    Blank line before the if statement.

    Instead of os.getcwd(), it's best to be explicit about file paths. You also don't want to assume path separators (which is the whole reason to use os.path.join). This should look more like:

    os.path.join(os.path.dirname(__file__), 'djblets', 'settings.py' ....)
    
  6. tests/runtests.py (Diff revision 1)
     
     
     

    We use parens for multi-line if statements. This should be more like:

    if (os.path.exists(...) and
        (os.stat(...) & ...)):
    

    Note also that you shouldn't need to cast to bool.

  7. 
      
TO
david
  1. 
      
  2. tests/runtests.py (Diff revision 2)
     
     
     
     
     

    You have some trailing whitespace on these lines (where it highlights in red in the diff viewer).

  3. tests/runtests.py (Diff revision 2)
     
     
     

    Can you indent 'settings.py' to line up with os.path.dirname?

  4. 
      
TO
david
  1. Ship It!

  2. 
      
TO
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (3267eb4). Thanks!
Loading...