Backport patches for Bloodhound commit ticket updater component

[Olemis Lang <olemis+trac@…>]

While implementing Bloodhound multi-product version of the commit ticket updater component​ we have produced some patches suitable to be backported into Trac . These are :

• portions of [bh:/attachment/ticket/695/t695_r1534486_commit_ticket_update.tests.func.diff] implementing functional test cases for this feature.
  • extend the signature of functional test env's svn_* methods.
• [bh:attachment/ticket/695/t695_r1545950_twill_write_html.diff t695_r1545950_twill_write_html.diff] which is aimed at logging failure conditions for test case classes defining local setUp and tearDown methods, like those included in patch mentioned above and in ​XmlRpcPlugin test suite.

Functional test case classes in Trac test suite mostly execute runTest method , nevertheless RPC test cases as well as those proposed in the patches use a class-level fixture initialization i.e. test_* methods wrapped in setUp and tearDown . Should many of these fail while executing a test run then twill output a single log file because the class name is the same for all instances and subsequent generations will be overwritten . That's what the later patch is all about .

[Olemis Lang <olemis+trac@…>]

For further details see ​bh:ticket:695

[Ryan J Ollos]

Replying to Olemis Lang <olemis+trac@…>:

• portions of [bh:/attachment/ticket/695/t695_r1534486_commit_ticket_update.tests.func.diff] implementing functional test cases for this feature.
  • extend the signature of functional test env's svn_* methods.

The patch included changes for one SvnFunctionalTestEnvironment method, which were committed to 1.0-stable in [12434] and merged to trunk in [12435].

• [bh:attachment/ticket/695/t695_r1545950_twill_write_html.diff t695_r1545950_twill_write_html.diff] which is aimed at logging failure conditions for test case classes defining local setUp and tearDown methods, like those included in patch mentioned above and in ​XmlRpcPlugin test suite.

The change to better_twill.html is staged in proposed changes for #10018.

Functional test case classes in Trac test suite mostly execute runTest method , nevertheless RPC test cases as well as those proposed in the patches use a class-level fixture initialization i.e. test_* methods wrapped in setUp and tearDown.

I took the same approach in the proposed changes for #10018. I think it may be a useful tool to reduce duplicated code in the functional tests by isolating common code for a set of tests to setUp and tearDown. Grouping related tests into classes can also help with organization of the tests.

Next I'll looking into backporting the actual functional tests.

[Ryan J Ollos]

Change to better_twill.html committed to 1.0-stable in [12452] and merged to trunk in [12455].

[Olemis Lang <olemis+trac@…>]

I've added an ​initial implementation in my patch queue but it's mysteriously failing /me testing …

[Olemis Lang <olemis+trac@…>]

Replying to Olemis Lang <olemis+trac@…>:

I've added an initial implementation in my patch queue but it's mysteriously failing /me testing …

It's working now in ​this version.

$ python trac/versioncontrol/tests/commit_updater.py
...
---------------------------------------------
Ran 3 tests in 104.211s

OK

[Ryan J Ollos]

I've done some work on the patch in log:rjollos.git:t11440.

Part of the patch in #11448 is needed to avoid test failures,

trac/tests/functional/testenv.py

@@ -68 +68 @@
         self.dirname = os.path.abspath(dirname)
         self.tracdir = os.path.join(self.dirname, "trac")
         self.htpasswd = os.path.join(self.dirname, "htpasswd")
+        self.trac_src = os.path.abspath('.')
         self.port = port
         self.pid = None
         self.init()
@@ -76 +77 @@
         self.create()
         locale.setlocale(locale.LC_ALL, '')
 
-    trac_src = '.'
-
     @property
     def dburi(self):
         dburi = get_dburi()

I have a few comments:

• We might consider using branches/1.0-stable/contrib/trac-svn-hook​ in the test.
• The test execution time is way too long for including this in the suite. Most of the time is spent creating the 27 tickets. There are a few ways I can think to deal with this:
  • Directly insert the tickets into the database.
  • Reduce the number of commands that are tested, and reply on unit tests for testing correct parsing of all the other commands: branches/1.0-stable/tracopt/ticket/tests/commit_updater.py​.
• So as to not leave minor TODOs in the ticket, the TODO: Find status=closed in UI should be addressed.

[Jun Omae]

I worked in log:jomae.git:t11440.1 to support Windows.

Also self._testenv.get_config('trac', 'repository_sync_per_request') wrongly returns the following value. [d816f47ae/jomae.git] and [910823fe/jomae.git] will fix it.

'(default)\r\nTrac [C:\\usr\\src\\trac\\trac.git\\testenv\\trac]> \r\n'

Next, post-commit hook on Linux cannot load trac/admin/console.py. [975845cf/jomae.git] will fix it.

/home/jun66j5/venv/py26/bin/python: can't open file './trac/admin/console.py': [Errno 2] No such file or directory

[Ryan J Ollos]

After rebasing on the changes applied in #11458 onto Jun's branch I get:

...
----------------------------------------------------------------------
Ran 3 tests in 41.551s

OK

I think we should aim to further reduce the execution time of the tests before committing the changes.

[Olemis Lang <olemis+trac@…>]

Replying to rjollos:

I've done some work on the patch in log:rjollos.git:t11440.

Part of the patch in #11448 is needed to avoid test failures,

[…]

Yes, that's related to issues mentioned by jomae .

<ot> Nevertheless I'd suggest not to use '.' as the path to trac source code . Recall that Bloodhound extends Trac test cases considering multi-product support, i.e. the test suite might be actually run from /path/to/bloodhound/bloodhound_multiproduct/ instead and trac source code would be /path/to/bloodhound/trac . Nevertheless afaicr BH test code will deal with this . </ot>

I have a few comments:

• We might consider using branches/1.0-stable/contrib/trac-svn-hook​ in the test.

0+

• The test execution time is way too long for including this in the suite. Most of the time is spent creating the 27 tickets.

yes … :-/

There are a few ways I can think to deal with this:

• Directly insert the tickets into the database.

If we are allowed to cheat then sure we can , though it'd be possible

• to reuse previous tickets created during the test run
• to create tickets once and propagate to subsequent instances via test_case.fixture or test_case.tester

• Reduce the number of commands that are tested, and reply on unit tests for testing correct parsing of all the other commands: branches/1.0-stable/tracopt/ticket/tests/commit_updater.py​.

I do not think this will speed up things too much as compared to the benefits of covering most cases. Most of the time is spent on ticket ops (… and indirect/inefficient twill matching …). There's a single changeset with refs to multiple tickets ⇒ IMO should be fast

• So as to not leave minor TODOs in the ticket, the TODO: Find status=closed in UI should be addressed.

Oh yes ! I forgot, sorry . I'll suggest something asap in a new patch .

[Olemis Lang <olemis+trac@…>]

Replying to Olemis Lang <olemis+trac@…>:

Replying to rjollos:

[…]

• Reduce the number of commands that are tested, and reply on unit tests for testing correct parsing of all the other commands: branches/1.0-stable/tracopt/ticket/tests/commit_updater.py​.

I do not think this will speed up things too much as compared to the benefits of covering most cases. Most of the time is spent on ticket ops (… and indirect/inefficient twill matching …). There's a single changeset with refs to multiple tickets ⇒ IMO should be fast

BTW , I forgot to mention that the original patches proposed for Bloodhound included unit tests like those you refer to only parse commit messages.

[…]

[Olemis Lang <olemis+trac@…>]

Replying to jomae:

I worked in log:jomae.git:t11440.1 to support Windows.

awesome ! I had no Window dev machine prepared to try it on that platform .

[…]

[Olemis Lang <olemis+trac@…>]

Replying to Olemis Lang <olemis+trac@…>:

Replying to rjollos:

[…]

• The test execution time is way too long for including this in the suite. Most of the time is spent creating the 27 tickets.

yes … :-/

There are a few ways I can think to deal with this:

• Directly insert the tickets into the database.

If we are allowed to cheat then sure we can ,

hmmm , I've been thinking about this a bit more and it's not as simple as I thought in first place . There is a complication related to keeping track of global ticket count and the added complexity when updating test cases for Bloodhound (e.g. unique ticket seq num for PostgreSQL vs product-speific ticket seq otherwise) .

[…] though it'd be possible

• to reuse previous tickets created during the test run
• to create tickets once and propagate to subsequent instances via test_case.fixture or test_case.tester

• to use a lightweight version of create_ticket with little overhead caused by twill.

[…]

[Olemis Lang <olemis+trac@…>]

Replying to rjollos:

After rebasing on the changes applied in #11458 onto Jun's branch I get:

...
----------------------------------------------------------------------
Ran 3 tests in 41.551s

OK

After checking out that branch early today I'm getting failures in both test_envelope and test_no_envelope . There's a mismatch since commit message is not appended in comments list of the 20th ticket .

[…]

[Olemis Lang <olemis+trac@…>]

Replying to Olemis Lang <olemis+trac@…>:

Replying to rjollos:

[…]

• So as to not leave minor TODOs in the ticket, the TODO: Find status=closed in UI should be addressed.

Oh yes ! I forgot, sorry . I'll suggest something asap in a new patch .

I've added a ​new patch for this , please review .

[Olemis Lang <olemis+trac@…>]

I've added ​another patch to backport test cases for parsing ticket refs commands in commit messages .

$ PYTHONPATH=. python tracopt/ticket/tests/commit_updater.py 
....
----------------------------------------------------------------------
Ran 4 tests in 0.082s

OK

please see ​patch order .

[Ryan J Ollos]

Replying to jomae:

Also self._testenv.get_config('trac', 'repository_sync_per_request') wrongly returns the following value. [d816f47ae/jomae.git] and [910823fe/jomae.git] will fix it.

'(default)\r\nTrac [C:\\usr\\src\\trac\\trac.git\\testenv\\trac]> \r\n'

These changes were committed in [12486:12487]. See comment:3:ticket:11472.

[ilewismsl]

Any success in duplicating the problem with permissions described on the Trac Development group: ​Defect in commit_updater permission checks when using ignore_auth_case.

[Ryan J Ollos]

As noted in comment:4:ticket:11559, I'll add a test case for the behavior that was fixed in #11559.

[Ryan J Ollos]

Narrowing focus for milestone:1.2. Please move ticket to milestone:1.2 if you intend to fix it.

[Ryan J Ollos]

Closing since tests to be backported run too slowly. We should write test coverage using unit tests.