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

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


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/

/Volumes/Augie/solarmist/rbtools/lib/python2.7/site-packages/nose/ 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/ 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/ 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/ DeprecationWarning: buffer() not supported in 3.x
#2 Testing the cache with a zero max-age value ... /Volumes/Augie/solarmist/rbtools/rbtools/api/ DeprecationWarning: buffer() not supported in 3.x
sqlite3.Binary(entry.response_body), entry.url,
Ran 210 tests in 204.074s

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

  • 0
  • 0
  • 9
  • 1
  • 10
Description From Last Updated
  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/ (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/ (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.

  2. Again, you do not need to include the entire test output.

  2. rbtools/clients/tests/ (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/ using globals.

      I'm removing it.

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

    Sentences in comments should end in a period.

  4. rbtools/clients/tests/ (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/ (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.

  2. rbtools/clients/tests/ (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/ (Diff revision 7)

    This should fit on one line.

  4. rbtools/utils/ (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.

Review request changed

Status: Discarded

Change Summary:

Discarded in favor of /r/9895/