Coverage Annotation is base on the last revision on the file, and not on the last revision of the repository

[ulrich.vdh@…]

Flow :

• I click on the Builds Status menu
• I select a build
• I select a file in the coverage section

The last modification of the file date of revision 1500. The selected revision in the builds status menu, is 2000. The annotate="coverage" commande show the coverage of the revision 1500 and not of the revision 2000.

If unit test change beetwen revision 1500 and 2000, the line cover, and the line not cover aren't take in consideration.

If before the revision 1750, no coverage where made. If i select revision 2000 and the file is not modified. The annotation is empty.

The annotate="coverage" command should be show the revision selected and not the last modified revision of the file.

Thanks.

[osimons]

Hmm. What is your Trac version? Something seems to have changed at some stage during Trac 0.11.x in how the currently browsed version is fetched. #448 has more about this issue.

[ulrich.vdh@…]

I have the last svn version of bitten and the version 0.11.1 of trac

[jerith <firxen+bitten@…>]

In the current trunk (approximately 0.6b2, I believe), the information is all present and correct, but displayed in a misleading way.

The coverage information displayed is that of the most recent revision, but the last-changed revision of the file whose coverage is being looked at is in bold and linked at the top of the coverage display block. Since coverage is an artifact of the revision as a whole rather than the revision of the file displayed, this should probably be dropped and the current repository revision used instead. It'll have to be cunningly worded to make it clear what the number actually represents.

I'm unfamiliar with the code, so I have no idea how big a change might be required for this, but I'll poke around and see what I can come up with.

[jerith <firxen+bitten@…>]

All the stuff related to that display is hardcoded in the trac template (trac/versioncontrol/browser.html). There may be a way to override just the header section, but that will be messy and may leave other stuff in an inconsistent state.

I think the fundamental problem is that the browser view assumes that one is interested in the file being displayed, whereas the coverage annotation is actually displaying information about the repository as a whole (the tests, at least, but also code that calls stuff in that file) applied to the file being displayed.

As a consequence, many of the surrounding links and displays reference the most recent changeset for the current file rather than the current repository revision. Specifically, the "Last Change" and "Normal" links point to the last-changed revision while the "Revision Log" link points to the current repo revision.

I don't really know where to go with this. The easy "solution" is to document the caveats around using this annotation. The best "real" solution I can think of requires rearchitecting how the browser view works in trac, which is probably infeasible for now.

[osimons]

Not essential for 0.6, so moving to next minor revision.

[osimons]

No, this is actually bugging me too much. Needs fixing now.

[osimons]

In [929] (and [930] for 0.6) I've made a fix. I think part of this ticket has been fixed before when we tweaked revision numbers, and the bug that I now hit was due to changes in how resource.id is used between Trac versions.

To summarize current status as I see it (testing using 0.12.1) - and reusing your revision number examples:

1. Linking from inside build status will link to the revision being built, which makes perfect sense. We want this revision whatever it was, and that works if such data exists.
2. the_file?annotate=coverage will default to latest for repository and will try to fetch coverage annotation for revision 2000. Correct so far, but of course depends on such a build existing - if it isn't built yet, we will not loop back to find the latest successful build >= revision 1500. That may be a very expensive check, and also remember that at revision 10000 you may have to loop all revisions in repository history and perhaps never find any coverage data... If we can't match it explicitly (or at latest), I don't think this will ever make sense.
3. Linking context navigation will produce the_file?annotate=coverage&rev=2000 link, which based on above produces the same result.
4. However, Trac 'annotate' and 'normal' linking switches the version which is not so nice. So if you use the Trac links to see blame information you are suddenly stuck at browsing revision 1500 and if you then switch to 'coverage' you will inherit this specific version. And Bitten has no way to distinguish this request from 1. above.

My feeling is that Bitten now does what it can, but that the Trac linking is wrong and only causes confusion. the_file?annotate=blame&rev=2000 produces exactly the same correct output as rev=1500 and Trac should keep same revision as well and not start jumping back in history.

Unless fresh insight is provided, I'm tempted to conclude that Bitten does what it can and that this issue is considered 'fixed'.

[anonymous]

Are there any relatively easy changes we could make in Trac to improve the situation? I'm thinking something along the lines of adding a repo-revision-bound VCS browser that can be used instead of the current file-revision-bound browser for things like this where we care about the revision as a whole.

(Disclaimer: I don't know any of this code at all at the moment, and my thinking is based on my previous comments on this ticket.)

[jerith <firxen+bitten@…>]

The above comment is mine. I forgot to set my username. :-/

[osimons]

Replying to osimons:

I think part of this ticket has been fixed before when we tweaked revision numbers...

That was #448, BTW.

[osimons]

Actually, case not quite closed. I've created ​trac:ticket:9694 with patch, and will hopefully get that applied to 0.12-stable at least. To go with that patch for Trac, here is the Bitten patch that uses same logic. Anyone want to test the patches and see if they make better sense?

Here is the corresponding Bitten patch:

bitten/report/coverage.py

@@ -194 +194 @@
                     title='Annotate file with test coverage '
                           'data (if available)',
                     href=req.href.browser(resource.id, 
-                        annotate='coverage', rev=resource.version))
+                        annotate='coverage', rev=req.args.get('rev')))
         return template, data, content_type
 
     # IHTMLPreviewAnnotator methods

[osimons]

Redone as all SQL. Seems to work, and should be quite a bit more efficient. Lots of test fail - still needs som Mock()'ing.

[osimons]

attachment:t466-annotate_link_args_rev_sql-r929.diff​ is a reimplementation using SQL only. A few things of note:

1. The context menu linking uses rev= only if it is currently part of args. If the get() returns None it will automatically be stripped from output. In addition a new argument is added to pass the created revision for last file change - to save a lookup when rendering (data is already available from browser).
2. It gathers a range of possible revisions from current revision being browsed and back to last changeset for the file being viewed.
3. For order, this range is mapped to the timestamps of the revisions used for the query.
4. A few JOIN's of the tables we need, and then looking for a combined concatenated pattern of config+/+file - typically like trunk+/+bitten/model (with + for illustration).
5. It orders with most recent first, and returns the first and only needed match.

It solves every possible issue with coverage annotation - as long as there is a build that stored coverage data since last file change.

Should a warning be added if no coverage information is found (as opposed to coverage found but no lines covered)?

[osimons]

Updated SQL for review:

cursor.execute("""
        SELECT b.id, b.rev, i2.value
        FROM bitten_config AS c
            INNER JOIN bitten_build AS b ON c.name=b.config
            INNER JOIN bitten_report AS r ON b.id=r.build
            INNER JOIN bitten_report_item AS i1 ON r.id=i1.report
            INNER JOIN bitten_report_item AS i2 ON (i1.item=i2.item
                                            AND i1.report=i2.report)
        WHERE i2.name='line_hits'
            AND b.rev_time>=%s
            AND b.rev_time<=%s
            AND i1.name='file'
            AND """ + db.concat('c.path', "'/'", 'i1.value') + """=%s
        ORDER BY b.rev_time DESC LIMIT 1""" ,
    (created_time, version_time, resource.id.lstrip('/')))

Still works... Better Hodgestar?

[osimons]

The complete fix is applied in [933] (trunk) and merged to 0.6dev in [934]. I've updated b.e.o already, so browsing this project should give very satisfactory results.

The SQL-based approach should be quite efficient compared to pulling all the objects. And, for trivia, I added rel="nofollow" to the context navigation link so that the bots will (hopefully) ignore this file that provides no real information compared to regular source browsing.

Now, if only ​trac:ticket:9694 can get the green light...

[osimons]

Replying to osimons:

Now, if only ​trac:ticket:9694 can get the green light...

All done. I've committed the fix to Trac for latest 0.11, 0.12 and trunk. That should provide the means to get this issue working OK for all Bitten users regardless of Trac major version in use.

[cboos]

Bitten's Trac instance was upgraded to 0.11.8dev-​r10236.