Fix unit tests to not pollute the test directories of other tests.

Review Request #9654 — Created Feb. 16, 2018 and updated

Joshua Olson
RBTools
master
9693
9655
fe2237f...
rbtools

Fix test pollution and unit tests.

What was happening was that a single test failure would cause all following tests to fail with directory not found when trying to modify the home directory.
This also uncovered an issue with test_diff_exclude_in_subdir and test_diff_exclude_root_pattern_in_subdir

Also I'm removing the mktemp since it is deprecated and the class already provides that functionality.

time ./tests/runtests.py

/Volumes/Augie/solarmist/rbtools/lib/python2.7/site-packages/nose/pyversion.py:36: DeprecationWarning: The 'new' module has been removed in Python 3.0; use the 'types' module instead.
import new
/Volumes/Augie/solarmist/rbtools/lib/python2.7/site-packages/nose/plugins/prof.py:14: DeprecationWarning: The 'hotshot' module is not supported in 3.x, use the 'profile' module instead.
import hotshot
/Volumes/Augie/solarmist/rbtools/lib/python2.7/site-packages/nose/pyversion.py:49: DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
class Key(object):
#1 Testing the cache with a high max-age value ... /Volumes/Augie/solarmist/rbtools/rbtools/api/cache.py:479: DeprecationWarning: buffer() not supported in 3.x
sqlite3.Binary(entry.response_body)))
ok
#2 Testing the cache with a zero max-age value ... /Volumes/Augie/solarmist/rbtools/rbtools/api/cache.py:493: DeprecationWarning: buffer() not supported in 3.x
sqlite3.Binary(entry.response_body), entry.url,
ok
...
Ran 210 tests in 204.074s

OK
./tests/runtests.py 85.38s user 56.70s system 69% cpu 3:24.53 total

  • 0
  • 0
  • 9
  • 1
  • 10
Description From Last Updated
Joshua Olson
Christian Hammond
  1. I feel like this change is perhaps trying to do too much. There's a cleanup of sorts with the home directory, but then a rewrite of some of the Bazaar tests, and other misc. changes. It's best to keep review requests focused, tackling a single thing, to help with review and to help to locate the causes of regressions later on.

    1. These are all very related. I was having tons of test failures because of test pollution. Each set of tests was moving directories around in their own ways. This makes all directory manipulation happen through the testbase class.

      The only pieces not directly related to that is changing a variable name to not use a keyword and making FOO strings for bzr.

    2. Makes sense. Still, breaking off the home directory cleanup from the Bazaar cleanup would make sense, so that each is focused on a task. I don't imagine the Bazaar cleanup is necessary for the home directory cleanup.

    3. Yup, that's what I did. I also refactored a bit more and I think it's cleaner now.

  2. rbtools/utils/testbase.py (Diff revision 1)
     
     
     

    Blank line between these.

    Mind also fixing the ending bit of the docstring? The """ was supposed to be on its own line.

  3. rbtools/utils/testbase.py (Diff revision 1)
     
     
     
     
     
     
     
     

    I'd like the docstrings to go into more detail on the reasons they'd be used. As it is, I'm not even clear.

    1. Will do. I needed the reeset_user_home function because of test pollution going on causing a failure in one test file to cause all remaining client unit tests to fail. The other one I can drop, but seemed like it might be useful for subclassed tests in the future. Like hg-svn after hg tests and git-svn tests following git tests. I can drop that functionality if you'd like.

  4. 
      
Joshua Olson
Joshua Olson
Joshua Olson
Barret Rennie
  1. 
      
  2. Again, you do not need to include the entire test output.

  3. 
      
Joshua Olson
Joshua Olson
David Trowbridge
  1. 
      
  2. rbtools/clients/tests/__init__.py (Diff revision 5)
     
     

    I'm not sure how this actually works. If we don't call RBTestBase.setUp(), self._old_cwd will never be defined, which is used in the (non-overridden) tearDown() method.

    1. This isn't used. It was used in a previous iteration of the code, instead I copied the pattern from rbtools/utils/filesystem.py using globals.

      I'm removing it.

  3. rbtools/clients/tests/__init__.py (Diff revision 5)
     
     

    Sentences in comments should end in a period.

  4. rbtools/clients/tests/test_git.py (Diff revision 5)
     
     

    Why was this moved? Can you clarify the motivation?

    1. Reverted and adding a comment.

      It was moved to make it more clear what it was related to, but I tihnk a comment is a better choice.

  5. rbtools/clients/tests/test_mercurial.py (Diff revision 5)
     
     

    This seems unrelated to the rest of the change.

    1. It's relvenat to fixing up the tests because running svnserve single threaded makes testing/debugging issues more straightforward.

  6. 
      
Joshua Olson
Joshua Olson
David Trowbridge
  1. 
      
  2. rbtools/clients/tests/__init__.py (Diff revision 7)
     
     
     

    This should fit on one line (just barely)

    1. These comments are not useful at all. Having a consistent style is one thing, but being pedantic is another.

      Code reviews and style guides are not meant to force every commit to have identical and perfect style.

  3. rbtools/clients/tests/__init__.py (Diff revision 7)
     
     
     

    This should fit on one line.

  4. rbtools/utils/testbase.py (Diff revision 7)
     
     
     
     

    There's some weird spelling/grammar in here. Might be nice to rewrite but can you at least change "suit" to "suite"?

    1. Agreed. I've re-worded it.

  5. 
      
Joshua Olson
Joshua Olson
Review request changed

Change Summary:

Fixed two unit tests that were failing once I untangled everything.

Description:

~  

Fix test pollution.

  ~

Fix test pollution and unit tests.

   
~  

What was happening was that a single test failure would cause all following tests to fail with directory not found when trying to modify the home directory.

  ~

What was happening was that a single test failure would cause all following tests to fail with directory not found when trying to modify the home directory.

  + This also uncovered an issue with test_diff_exclude_in_subdir and test_diff_exclude_root_pattern_in_subdir

   
   

Also I'm removing the mktemp since it is deprecated and the class already provides that functionality.

Commit:

-ecc239a77c4576312d081e8f4a9e0bff6f3aaf84
+fe2237fefb51d20f715682532afc6902a6cc68f7

Diff:

Revision 9 (+104 -35)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...