Edgewall Software
Modify

Opened 10 years ago

Closed 7 years ago

#11440 closed enhancement (wontfix)

Backport patches for Bloodhound commit ticket updater component

Reported by: Olemis Lang <olemis+trac@…> Owned by:
Priority: low Milestone:
Component: ticket system Version: 1.0.1
Severity: minor Keywords: patch vcs hooks bloodhound backport CommitTicketUpdater functional-tests
Cc: Jun Omae Branch:
Release Notes:
API Changes:
Internal Changes:

Description (last modified by Ryan J Ollos)

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 .

Attachments (0)

Change History (26)

comment:1 by Olemis Lang <olemis+trac@…>, 10 years ago

For further details see bh:ticket:695

comment:2 by Ryan J Ollos, 10 years ago

Description: modified (diff)

in reply to:  description comment:3 by Ryan J Ollos, 10 years ago

Component: generalticket system
Keywords: functional tests CommitTicketUpdater added
Milestone: 1.0.2
Owner: set to Ryan J Ollos
Status: newassigned

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.

comment:4 by Ryan J Ollos, 10 years ago

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

comment:5 by Olemis Lang <olemis+trac@…>, 10 years ago

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

in reply to:  5 comment:6 by Olemis Lang <olemis+trac@…>, 10 years ago

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

comment:7 by Ryan J Ollos, 10 years ago

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

    diff --git a/trac/tests/functional/testenv.py b/trac/tests/functional/testenv.py
    index ec3cc54..b9e09ef 100755
    a b class FunctionalTestEnvironment(object):  
    6868        self.dirname = os.path.abspath(dirname)
    6969        self.tracdir = os.path.join(self.dirname, "trac")
    7070        self.htpasswd = os.path.join(self.dirname, "htpasswd")
     71        self.trac_src = os.path.abspath('.')
    7172        self.port = port
    7273        self.pid = None
    7374        self.init()
    class FunctionalTestEnvironment(object):  
    7677        self.create()
    7778        locale.setlocale(locale.LC_ALL, '')
    7879
    79     trac_src = '.'
    80 
    8180    @property
    8281    def dburi(self):
    8382        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:
  • So as to not leave minor TODOs in the ticket, the TODO: Find status=closed in UI should be addressed.

comment:8 by Jun Omae, 10 years ago

Cc: Jun Omae added

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

comment:9 by Ryan J Ollos, 10 years ago

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.

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

in reply to:  7 ; comment:10 by Olemis Lang <olemis+trac@…>, 10 years ago

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:

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

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 .

in reply to:  10 comment:11 by Olemis Lang <olemis+trac@…>, 10 years ago

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

Replying to rjollos:

[…]

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.

[…]

in reply to:  8 comment:12 by Olemis Lang <olemis+trac@…>, 10 years ago

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 .

[…]

in reply to:  10 comment:13 by Olemis Lang <olemis+trac@…>, 10 years ago

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.

[…]

in reply to:  9 comment:14 by Olemis Lang <olemis+trac@…>, 10 years ago

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 .

[…]

in reply to:  10 comment:15 by Olemis Lang <olemis+trac@…>, 10 years ago

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 .

comment:16 by Olemis Lang <olemis+trac@…>, 10 years ago

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 .

in reply to:  8 comment:17 by Ryan J Ollos, 10 years ago

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.

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

comment:18 by Jun Omae, 10 years ago

Keywords: functional-tests added; functional tests removed

comment:19 by ilewismsl, 10 years ago

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.

comment:20 by Ryan J Ollos, 10 years ago

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

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

comment:21 by Ryan J Ollos, 10 years ago

Milestone: 1.0.21.0.3

comment:22 by Ryan J Ollos, 9 years ago

Milestone: 1.0.3next-stable-1.0.x
Owner: Ryan J Ollos removed
Status: assignednew

comment:23 by Ryan J Ollos, 9 years ago

Milestone: next-stable-1.0.xnext-dev-1.1.x

comment:24 by Ryan J Ollos, 9 years ago

Milestone: next-dev-1.1.xnext-dev-1.3.x

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

comment:25 by figaro, 8 years ago

Keywords: patch added

comment:26 by Ryan J Ollos, 7 years ago

Milestone: next-dev-1.3.x
Resolution: wontfix
Status: newclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The ticket will remain with no owner.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none) to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.