filling in features and tests for hg and hg+svn
Review Request #1464 — Created March 8, 2010 and submitted
Information | |
---|---|
dbuch-agi | |
RBTools | |
Reviewers | |
rbtools, reviewboard | |
chipx86, david, durin42, mcrute |
We're using ReviewBoard quite a bit now at AG Interactive and are beginning to experiment with Mercurial as our primary SCM/VCS/whatever instead of Subversion. Initial experiments with `post-review` and pure-Mercurial repos (not hgsubversion clones) have indicated that `post-review` is geared toward use with hgsubversion clones. This patch seeks to do the following: - fixes the assumption that `hg svn info` returning non-error means the working copy is an hgsubversion clone. - adds support for reading 'reviewboard.url' option from local hgrc (global hgrc support unimplemented) - adds support for posting reviews of locally-committed changes (old behavior only supported posting uncommitted changes) - adds support for `--parent` option to specify remote for outgoing diff - add tests for both hg and hg+svn client stuff
ran existing tests, wrote and ran new tests (included in patch)
-
-
-
rbtools/postreview.py (Diff revision 1) These should be defined in the constructor, not as part of the class.
-
rbtools/postreview.py (Diff revision 1) Any constants should be ALL_UPPERCASE, with no leading _. The comment should be aligned with the variable name, not the contents. Also, please describe why the order counts, and the purpose for the constant.
-
rbtools/postreview.py (Diff revision 1) Can the call to _load_hgrc just be in the get_repository_info function?
-
-
-
-
rbtools/postreview.py (Diff revision 1) Multi-line conditionals should be inside a (...) instead of using \
-
-
-
-
-
rbtools/postreview.py (Diff revision 1) I prefer more explicit, spelled-out statements, like: if files is None: files = [] Avoid the assignment unless necessary.
-
rbtools/postreview.py (Diff revision 1) It'd be nice to have a documentation block for this function. Really, same with any new functions we add.
-
-
-
rbtools/postreview.py (Diff revision 1) Can you put the execute line on its own line and store the value? Then iterate over that.
-
rbtools/postreview.py (Diff revision 1) No ^--. Comments like this should precede the line they reference. Are branch names with whitespace at all common, or even allowed?
-
rbtools/postreview.py (Diff revision 1) Can you add a comment explaining what this does for future maintainers? It's a slightly complex filter if you haven't dealt with ifilter and don't know the input exactly.
-
rbtools/postreview.py (Diff revision 1) Please spell this out as: if branch_name: branch_name = .... else: branch_name = ....
-
-
-
-
rbtools/postreview.py (Diff revision 1) See if this wraps better: server_url = \ super(...).scan_for_server(...)
-
rbtools/postreview.py (Diff revision 1) I don't like doing the 'if server_url' each time. Please do: if (not server_url and self....): server_url = ... if not server_url and self._type == 'svn': server_url = SVNClient()..... return server_url
-
-
rbtools/tests.py (Diff revision 1) We shouldn't use nose's assertion functions. We should be using the standard unittest's versions.
-
-
rbtools/tests.py (Diff revision 1) This blank line should be removed. Also, this object should inherit from unittest.TestCase.
-
rbtools/tests.py (Diff revision 1) Tests shouldn't need to operate against a live server, or we're going to hit issues in nightly builds.
-
rbtools/tests.py (Diff revision 1) Test functions should all be in testCamelCase style, and should have a one-line docstring explaining what's being tested, in the form of: """Test MercurialClient get_repository_info""" I know not all tests in this file do it. We need to fix this. It's inconsistent with all the other unit tests in Review Board.
-
rbtools/tests.py (Diff revision 1) We seem to do this in every test. Can we do it from setUp() instead?
-
-
rbtools/tests.py (Diff revision 1) We shouldn't be treating paths with manual string processing. Use the os.path functions, like os.path.basename and os.path.dirname.
-
-
-
rbtools/tests.py (Diff revision 1) These are all class-global. They should be defined in setUp(). Same with other classes.
-
-
rbtools/tests.py (Diff revision 1) I'm not a fan of any(). It's less explicit. Please do: for exe in ["svnadmin", "svnserve", "svn"]: if not is_exe_in_path(exe): raise nose.SkipTest('Cannot test Mercurial Subversion integration: Missing %s" % exe)
-
-
rbtools/tests.py (Diff revision 1) Should include the exception information in the SkipTest so people can debug why it's not working.
-
-
-
rbtools/tests.py (Diff revision 1) If this fails, we should know why. If it could fail because the path doesn't exist, we should test for the existence first.
-
-
-
rbtools/tests.py (Diff revision 1) This doesn't apply just to this function, but all private functions should start with __.
-
rbtools/tests.py (Diff revision 1) Should probably be done from setUp, if we always need it. Also, functions should never start with "get" unless they return a value.
-
-
only a few points of clarification needed, methinks. next draft of diff on its way.
-
rbtools/tests.py (Diff revision 1) fwiw, nose.tools.assert* are just aliases to unittest.TestCase.assert* with PEP8 names, but I'm going to switch to unittest.TestCase anyway for style continuity
-
rbtools/tests.py (Diff revision 1) We (at AGI) tend to prefer no docstrings for tests so that the fully qualified test name shows in verbose nosetests output, but I'll add docstrings for style continuity.
-
rbtools/tests.py (Diff revision 1) but ... it's *private*, not *protected*. I have always operated under the impression that using leading __ was BAD BAD BAD unless *absolutely* *necessary* because of the nonsensical name mangling that's done as a result :-/
-
rbtools/tests.py (Diff revision 1) how do you mean, "combined" ? :-/ ... is it an indentation issue? line wrapping incorrect?
Description: |
|
---|
-
BUMP again Until this is reviewed (and accepted?) the folks at my workplace will have to continue installing RBTools from my fork on github instead of just letting pip do its job :-/ Pretty please?
-
A few more things from this pass. I think it's fine after this.
-
rbtools/postreview.py (Diff revision 3) We create a RepositoryInfo several times here. Maybe do: path = self.hg_root base_path = '/' if self.has_hgrc: self._calculate_remote_path() if self._remote_path: path = self._remote_path[1] base_path = '' return RepositoryInfo(path=path, base_path=base_path, supports_parent_diffs=True)
-
-
rbtools/postreview.py (Diff revision 3) We never seem to actually do anything with this variable. Can we just call self.hgrc.read() without a return value? Or do we need this line at all?
-
-
-
-
-
rbtools/postreview.py (Diff revision 3) Just use the @classmethod decorator on the function directly. What's the reason for needing a classmethod though?
-
rbtools/postreview.py (Diff revision 3) Can you align this like: if (not server_url and (self.has_hgrc and self.....)):
Change Summary:
changes per feedback I tried to provide comments explaining my reasoning here and there :) The diff of diffs r3 and r4 does not accurately reflect changes I made because I rebased against current origin/master in the meantime. The summary diff (r4) *is* reflective of the work I've done.
Diff: |
Revision 4 (+754 -116) |
---|
-
This looks fine, but I'm getting three unit test failures. Specifically: FAIL: Test MercurialClient diff with diverged branch FAIL: Test MercurialClient diff, simple case FAIL: Test MercurialClient diff with multiple commits I have Mercurial 1.3.1. Are you seeing this at all? What version are you running?
-
Hi, someone found me in the #mercurial channel and I figured I'd chime in with my 2¢ (I wrote the hgsubversion bits back when I was at my previous company and used svn). In general, I'd encourage you to take a look at 'hg showconfig' as that should let you safely parse the paths the same way hg would, including any potential include statements in the hgrc file. If you want to suppress any paths from the user's global ~/.hgrc (although I'm not sure you should suppress those), you can set the environment variable HGRCPATH to /dev/null in hg's environment.
-
rbtools/postreview.py (Diff revision 4) You might be happier using 'hg showconfig' or something similar to extract the variables you really need. We support things in Mercurial's config file parser that may not work in ConfigParser (we don't use ConfigParser internally).
-
rbtools/postreview.py (Diff revision 4) Why not just do a template like this: b:{branches}\n r:{rev}\n\n and then split revisions on the double newline and the rest on single newlines?
Change Summary:
changes per feedback (with a puzzling change in some CC-related stuff that seems to have tagged along since I pulled from canonical branch)
People: |
|
||
---|---|---|---|
Diff: |
Revision 5 (+763 -116) |
-
A couple of minor things, but the over uses of hg seem fine to me.
-
rbtools/postreview.py (Diff revision 5) Function decorators are available in 2.4, is 2.3 compatibility a concern?
-
-
rbtools/postreview.py (Diff revision 5) I'm not sure about this logic - what if the repo doesn't have an hgrc but the user specified a reviewboard.url in their global hgrc?
-
rbtools/tests.py (Diff revision 5) rather than the fragile \ operator, why not do .strip() on the end of the triple-quoted block?
Change Summary:
another round of changes per feedback from durin42 (thanks, durin42!) :)
Diff: |
Revision 7 (+756 -116) |
---|
-
LGTM, only one minor comment. I'll leave it to the reviewboard guys to review further since it's really their code, and I'm just a passer-by.
-
-
I don't know why, but I'm still hitting unit test failures. Notice the diff headers. ====================================================================== FAIL: Test MercurialClient diff with diverged branch ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/chipx86/src/rb/rbtools/rbtools/tests.py", line 532, in testDiffBranchDiverge self.assertEqual((EXPECTED_HG_DIFF_2, None), self.client.diff(None)) AssertionError: ('diff --git a/foo.txt b/foo.txt\n--- a/foo.txt\n+++ b/foo.txt\n@@ -1,3 +1,5 @@\n+ARMA virumque cano, Troiae qui primus ab oris\n+ARMA virumque cano, Troiae qui primus ab oris\n ARMA virumque cano, Troiae qui primus ab oris\n Italiam, fato profugus, Laviniaque venit\n litora, multum ille et terris iactatus et alto\n', None) != ('diff -r be86bd1cc883 -r d869bd392b11 foo.txt\n--- a/foo.txt\tFri May 21 01:28:59 2010 -0700\n+++ b/foo.txt\tFri May 21 01:28:59 2010 -0700\n@@ -1,3 +1,5 @@\n+ARMA virumque cano, Troiae qui primus ab oris\n+ARMA virumque cano, Troiae qui primus ab oris\n ARMA virumque cano, Troiae qui primus ab oris\n Italiam, fato profugus, Laviniaque venit\n litora, multum ille et terris iactatus et alto\n', None) -------------------- >> begin captured stdout << --------------------- >>> hg --cwd /tmp/tmpSRDOk8__rbtools_tests init >>> hg add foo.txt >>> hg commit -m "initial commit" >>> hg clone /tmp/tmpSRDOk8__rbtools_tests /tmp/tmpy8HeN3__rbtools_tests >>> hg showconfig >>> hg root >>> hg svn info >>> Using candidate path 'default': '/tmp/tmpSRDOk8__rbtools_tests' >>> repository info: Path: /tmp/tmpSRDOk8__rbtools_tests, Base path: , Supports changesets: False >>> hg add foo.txt >>> hg commit -m "commit 1" >>> hg branch diverged >>> hg add foo.txt >>> hg commit -m "commit 2" >>> hg showconfig >>> hg svn info >>> repository info: Path: /tmp/tmpSRDOk8__rbtools_tests, Base path: , Supports changesets: False >>> hg branch >>> hg -q outgoing --template b:{branches} r:{rev} default >>> Found outgoing changeset 2 for branch 'diverged' >>> hg diff -r 1 -r 2 --------------------- >> end captured stdout << ---------------------- ====================================================================== FAIL: Test MercurialClient diff, simple case ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/chipx86/src/rb/rbtools/rbtools/tests.py", line 510, in testDiffSimple self.assertEqual((EXPECTED_HG_DIFF_0, None), diff_result) AssertionError: ('diff --git a/foo.txt b/foo.txt\n--- a/foo.txt\n+++ b/foo.txt\n@@ -6,7 +6,4 @@\n inferretque deos Latio, genus unde Latinum,\n Albanique patres, atque altae moenia Romae.\n Musa, mihi causas memora, quo numine laeso,\n-quidve dolens, regina deum tot volvere casus\n-insignem pietate virum, tot adire labores\n-impulerit. Tantaene animis caelestibus irae?\n \n', None) != ('diff -r 47bb0a09fdfc -r 48b4dcabcef4 foo.txt\n--- a/foo.txt\tFri May 21 01:29:00 2010 -0700\n+++ b/foo.txt\tFri May 21 01:29:01 2010 -0700\n@@ -6,7 +6,4 @@\n inferretque deos Latio, genus unde Latinum,\n Albanique patres, atque altae moenia Romae.\n Musa, mihi causas memora, quo numine laeso,\n-quidve dolens, regina deum tot volvere casus\n-insignem pietate virum, tot adire labores\n-impulerit. Tantaene animis caelestibus irae?\n \n', None) -------------------- >> begin captured stdout << --------------------- >>> hg --cwd /tmp/tmpI7q20N__rbtools_tests init >>> hg add foo.txt >>> hg commit -m "initial commit" >>> hg clone /tmp/tmpI7q20N__rbtools_tests /tmp/tmpWVGDkR__rbtools_tests >>> hg showconfig >>> hg root >>> hg svn info >>> Using candidate path 'default': '/tmp/tmpI7q20N__rbtools_tests' >>> repository info: Path: /tmp/tmpI7q20N__rbtools_tests, Base path: , Supports changesets: False >>> hg showconfig >>> hg svn info >>> repository info: Path: /tmp/tmpI7q20N__rbtools_tests, Base path: , Supports changesets: False >>> hg add foo.txt >>> hg commit -m "delete and modify stuff" >>> hg branch >>> hg -q outgoing --template b:{branches} r:{rev} default >>> Found outgoing changeset 1 for branch 'default' >>> hg diff -r 0 -r 1 --------------------- >> end captured stdout << ---------------------- ====================================================================== FAIL: Test MercurialClient diff with multiple commits ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/chipx86/src/rb/rbtools/rbtools/tests.py", line 522, in testDiffSimpleMultiple self.assertEqual((EXPECTED_HG_DIFF_1, None), diff_result) AssertionError: ('diff --git a/foo.txt b/foo.txt\n--- a/foo.txt\n+++ b/foo.txt\n@@ -1,12 +1,11 @@\n+ARMA virumque cano, Troiae qui primus ab oris\n ARMA virumque cano, Troiae qui primus ab oris\n Italiam, fato profugus, Laviniaque venit\n litora, multum ille et terris iactatus et alto\n vi superum saevae memorem Iunonis ob iram;\n-multa quoque et bello passus, dum conderet urbem,\n+dum conderet urbem,\n inferretque deos Latio, genus unde Latinum,\n Albanique patres, atque altae moenia Romae.\n+Albanique patres, atque altae moenia Romae.\n Musa, mihi causas memora, quo numine laeso,\n-quidve dolens, regina deum tot volvere casus\n-insignem pietate virum, tot adire labores\n-impulerit. Tantaene animis caelestibus irae?\n \n', None) != ('diff -r 821c9698ff13 -r 53714ccbbd24 foo.txt\n--- a/foo.txt\tFri May 21 01:29:02 2010 -0700\n+++ b/foo.txt\tFri May 21 01:29:04 2010 -0700\n@@ -1,12 +1,11 @@\n+ARMA virumque cano, Troiae qui primus ab oris\n ARMA virumque cano, Troiae qui primus ab oris\n Italiam, fato profugus, Laviniaque venit\n litora, multum ille et terris iactatus et alto\n vi superum saevae memorem Iunonis ob iram;\n-multa quoque et bello passus, dum conderet urbem,\n+dum conderet urbem,\n inferretque deos Latio, genus unde Latinum,\n Albanique patres, atque altae moenia Romae.\n+Albanique patres, atque altae moenia Romae.\n Musa, mihi causas memora, quo numine laeso,\n-quidve dolens, regina deum tot volvere casus\n-insignem pietate virum, tot adire labores\n-impulerit. Tantaene animis caelestibus irae?\n \n', None) -------------------- >> begin captured stdout << --------------------- >>> hg --cwd /tmp/tmp5Okq3z__rbtools_tests init >>> hg add foo.txt >>> hg commit -m "initial commit" >>> hg clone /tmp/tmp5Okq3z__rbtools_tests /tmp/tmpfwL8Nz__rbtools_tests >>> hg showconfig >>> hg root >>> hg svn info >>> Using candidate path 'default': '/tmp/tmp5Okq3z__rbtools_tests' >>> repository info: Path: /tmp/tmp5Okq3z__rbtools_tests, Base path: , Supports changesets: False >>> hg showconfig >>> hg svn info >>> repository info: Path: /tmp/tmp5Okq3z__rbtools_tests, Base path: , Supports changesets: False >>> hg add foo.txt >>> hg commit -m "commit 1" >>> hg add foo.txt >>> hg commit -m "commit 2" >>> hg add foo.txt >>> hg commit -m "commit 3" >>> hg branch >>> hg -q outgoing --template b:{branches} r:{rev} default >>> Found outgoing changeset 1 for branch 'default' >>> Found outgoing changeset 2 for branch 'default' >>> Found outgoing changeset 3 for branch 'default' >>> hg diff -r 0 -r 3
Change Summary:
Found it. I wasn't making sure the testing clones had diff.git enabled. I simulated "vanilla" conditions (I hope) by moving all of my user-level and system-level hgrcs out of the way, re-ran the tests. I reproduced chipx86's failure reliably and then fixed 'er up.
Diff: |
Revision 9 (+758 -116) |
---|
Change Summary:
Running `hg` with env that includes HGRCPATH=/dev/null and HGPLAIN=1 in every case possible to avoid the unexpected (per suggestion from durin42)
Diff: |
Revision 10 (+793 -123) |
---|
-
BUMP I just had to spend yet-another-5-minutes explaining to a veteran developer that `post-review` will not play nice with our Mercurial setup, showing them to my forked version on github. Not cool. Pretty please review and *at least* give me a Ship It or wontfix or whatever so I know whether or not I should be officially forking RBTools :P
-
Thanks Dan. Really sorry about the wait on these. They just fell off my radar. Committed to master (9996339).
-
It's good that Mercurial support is being worked on, but at the company I work for, we have been doing reviews based on checked-in changesets for ages, using --revision-range. I guess I haven't wrapped my head around the new philosophy of doing this yet.
-
rbtools/postreview.py (Diff revision 10) This is not a good idea, because it takes away the ability to say: --revision-range=tip~1:tip (using the parentrevspec extension). I guess HGPLAIN alone should suffice.