[MERGE][#175520] use historical context for file logging

[Vincent Ladeuil]

>>>>> "Ian" == Ian Clatworthy <ian.clatworthy at internode.on.net> writes:

    Ian> Vincent Ladeuil wrote:
    >> Some comments on the discussions before the review itself.
    >> 
    >> The outcome is a BB:tweak but ask for some tests rewriting.

    Ian> Thanks on both counts. Apologies for the delayed response.

    >>>>>>> "Ian" == Ian Clatworthy <ian.clatworthy at internode.on.net> writes:
    >> 
    Ian> Adrian Wilkins wrote:
    >> >>> If we start supporting file-ids in the UI, I'd like to do so
    >> >>> across every command and allow you to specify "fileid:..."
    >> >>> anywhere you can currently specify a file.
    >> 
    >> I disagree, I can't think of another command that needs file-id
    >> like log does. Care to elaborate ?

    Ian> Sure. Right now, we use a 'revid:' prefix to give the internal
    Ian> revision-id instead of the "normal" revno. For consistency, I think
    Ian> we ought to treat file-handles the same, generally available way.

    Ian> Right now, log might be the driver behind wanting to
    Ian> specify a file-id.  It's not hard to imagine the need
    Ian> spreading though to commands like missing,

missing as nothing to do with file ids AFAICS >-/

    Ian> annotate, status -c and diff -c.

I can stretch my imagination for these ones, but the need if far
less important than for log, but anyway, the one implementing it
will have his word to say here, so let's not bike-shed :)

<snip/>

    >> Wow, that's really desperate :-) 

    Ian> Desperate but semantically consistent. :-)

Sure, that was implicit in the joke :)

    >> I frankly doubt it will often succeeds... but since failures to
    >> find the file id are trapped at the caller level, I'm fine with
    >> that.
    >> 
    >> As long as you add some direct tests for it (one more reason to
    >> put it in the log module).

    Ian> I haven't done this bit yet. It's currently a private
    Ian> (to bzrlib) API and isn't going to become a public
    Ian> one. In fact, it gets reworked a lot in my multiple
    Ian> file/directory logging patch. I'll most likely add some
    Ian> low level tests for the revised version in that
    Ian> branch. For now, *I* feel the higher level tests
    Ian> adequately cover the functionality.

My biggest problem with this approach (relying on higher levels
tests for low-level features) is two-fold:

- the low level feature test coverage is blurry and as time
  passes the understanding of that coverage decrease, bitrot, and
  bugs become to appear,

- it doesn't provide as much support for TDD in terms of
  incremental steps (test a little, code a little),

While the later may be subjective and a personal choice, the
former has already bitten me (us) several times.


    Ian> +    def test_log_file_historical(self):
    Ian> +        """File matched against revision range, not current tree."""
    Ian> +        self.prepare_tree(complex=True)
    Ian> +
    Ian> +        # Check logging a deleted file gives an error if the
    Ian> +        # file isn't found at the end or start of the revision range
    Ian> +        err_msg = "Path unknown at end or start of revision range: file2"
    Ian> +        err = self.run_bzr('log file2', retcode=3)[1]
    Ian> +        self.assertContainsRe(err, err_msg)
    Ian> +
    Ian> +        # Check logging a deleted file is ok if the file existed
    Ian> +        # at the end the revision range
    Ian> +        log, err = self.run_bzr('log -r..4 file2')
    Ian> +        self.assertEquals('', err)
    Ian> +        self.assertNotContainsRe(log, 'revno: 1\n')
    Ian> +        self.assertContainsRe(log, 'revno: 2\n')
    Ian> +        self.assertNotContainsRe(log, 'revno: 3\n')
    Ian> +        self.assertContainsRe(log, 'revno: 3.1.1\n')
    Ian> +        self.assertContainsRe(log, 'revno: 4\n')
    Ian> +
    Ian> +        # Check logging a deleted file is ok if the file existed
    Ian> +        # at the start of the revision range
    Ian> +        log, err = self.run_bzr('log file1')
    Ian> +        self.assertEquals('', err)
    Ian> +        self.assertContainsRe(log, 'revno: 1\n')
    Ian> +        self.assertNotContainsRe(log, 'revno: 2\n')
    Ian> +        self.assertNotContainsRe(log, 'revno: 3\n')
    Ian> +        self.assertNotContainsRe(log, 'revno: 3.1.1\n')
    Ian> +        self.assertNotContainsRe(log, 'revno: 4\n')
    >> 
    >> I'd really really prefer if we can define some dumb log formatter
    >> that just records the revisions it displays so that the tests can
    >> then use some 'rev in list' conditions.

    Ian> That would be nice.

JDI :)

    >> And this test is at least three different ones (you have the
    >> comment to give them a name), split it please.

    Ian> I did this as requested. I'm not totally convinced of the value of
    Ian> doing so because I *like* seeing the tests collected together like
    Ian> this, but it's not a big deal to me either way.

Defect localization is the main motivation, here, if the first
test fails you have no idea about what the two others can reveal.

If you want to group them, you can still put them one after the
other in the source and chose method names that reflect that
grouping.

IME, separating them often allows factorization which in turn
highlight helpers which finally make tests easier to write (as
opposed to copy/paste) hence easier to read.

        Vincent