strange test name ;-) z missing

Thanks. I changed it to caddrdb_read, since that is a better descriptor of what I am testing.

Could make sense to move the hash check to the outer read function - after all, it's not part of the inner stream but of the wrapping file.
Would simplify testing as there'd be no need to pass hashIn here.

My thinking with passing hashIn as a parameter was that it would allow me to write a test on the "inner read" to verify an error is thrown if hashIn is not the same as the hash of ssPeers. Something like:

BOOST_AUTO_TEST_CASE(caddrdb_hash)
{
    CAddrManUncorrupted addrmanUncorrupted;
    CService addr1 = CService("250.7.1.1", 8333);
    addrmanUncorrupted.Add(CAddress(addr1), CService("252.5.1.1", 8333));

    // Test that the de-serialization does not throw an exception.
    CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted);

    addrmanUncorrupted.Add(CAddress(addr3), CService("253.3.3.33", 8333));
    CDataStream ssPeersDiff = AddrmanToStream(addrmanUncorrupted);
    uint256 badHashIn = Hash(ssPeersDiff.begin(), ssPeersDiff.end());

    CAddrDB adb;
    CAddrMan addrman;
    adb.Read(addrman, ssPeers2, badHashIn); // Assert this throws an exception since the hashes don't match
}

but I decided to wait until I had more time to work on it.

How interested are you in a system for mocking out filesystem objects rather than having to do everything at the stream level?

Ok, yes, that makes sense.

How interested are you in a system for mocking out filesystem objects rather than having to do everything at the stream level?

Depends on the (non-test) code impact. I'm not entirely sure that is necessary - e.g. you could create a temporary file and make it read that.

Depends on the (non-test) code impact.

It is on my list of things to investigate, but I was considering creating a class which wraps File/fopen and can be mocked out in unitests similar to RandomInt in addrman.h.

https://github.com/bitcoin/bitcoin/blob/master/src/addrman.h#L239

I share your concern as this would require a big diff when all the instances of File are replaced with mockable File. I assume this is a problem others have solved so I'm looking into alternates.

I'm not entirely sure that is necessary - e.g. you could create a temporary file and make it read that.

YMMV, but in my experience tmp files in unittests cause hard to debug test failures due to filesystem weirdness and added state (file deletes sometimes fail).

YMMV, but in my experience tmp files in unittests cause hard to debug test failures due to filesystem weirdness and added state (file deletes sometimes fail).

I agree. It's somewhat preferable to not clutter files around the place.
On the other hand it actually tests the filesystem interaction then :)

I share your concern as this would require a big diff when all the instances of File are replaced with mockable File.

C++ has some support for this by replacing inputs/outputs streams by string streams, unfortunately - or not, I'm not entirely up to date of the pros and cons of it - we don't really use C++'s file API.

We mainly use fopen and FILE*. Note that on some platforms (including Linux and MacOSX IIRC) for FILE* there is: fmemopen, open_memstream. May be interesting.

Then one could change the APIs to take a FILE* instead of filename. On platforms without the function it could fall back to a temporary file, on platforms with the function it can use a memory stream.

adddrman