bloom_tests: Do not depend on specific serialisations #6700

No description provided.

ACK, I think it's a good idea to build the transactions in-place instead of hardcoding them.

@laanwj could we use a JSON format instead?

Relying on the test creator to intricately build a transaction each time can be more error prone then just adapting a human-readable data format to what is needed for the test case.

@dcousens Error-prone in what sense? Remember the purpose here is to find errors. :)

@luke-jr from my own experience in writing tests for https://github.com/bitcoinjs/bitcoinjs-lib, the most common source of regressions / uncaught errors was actually in the test being poorly written, or a simple syntax error causing something to be missed. This may have been symptomatic of how wonderful Javascript is as a language.

Adopting a data-driven approach for us meant we only had to validate one test function that built the test data, and then validated it, from a human readable data format. In our case, some of that data can be seen here.

The serialized transaction in its hex form is included for a byte-for-byte comparison, but it should be evident that the test data is verbose enough that you can easily read and add test fixtures.

In general I'm in favor of data-driven tests, as it alllows comparing between implementations.

But there is a good point that tests should not be too fragile, they should only break when something is wrong, so they should test what matters, not the e.g. an exact sequence of steps if only the outcome is relevant.

In the case of bloom filtering the goal is to test bloom filtering. Not transaction serialization/deserialization. If the transaction format changes (e.g. as in elements, or an altcoin), this test should ideally not break.

I'm not entirely convinced that data-driven tests will add something here.

@laanwj the alternative of in-lining all the TX construction code seems worse IMHO. Why not break it out to a JSON file to keep the tests streamline? (I may be underestimating how 'streamline' this will be for C++ however)

If this is wanted, then any reason not to merge?

I don't feel strongly enough about it to merge it. Certainly with just my ACK. Seems at least you have some concerns with it.

Needs input from different people.

@laanwj my concern is this makes it more difficult to port these tests to other libraries based on the serialized transactions alone. But it does make the tests more transparent as to what they might actually be testing.

@dcousens I don't think it makes these any more difficult than any other tests we already have?

@luke-jr well, the serialized JSON fixtures are usually as simple as a copy paste. These fixtures would mean I'd have to compile the code and generate the fixtures my self.

I'm comparing it with the other tests, not the current version of this test...

It doesn't seem there is strong enough consensus to make this change, so going to close this.