This splits the output comparison for bitcoin-tx into two steps:
For either of these cases give a different error.
This prevents wild goose chases when e.g. a trailing space doesn't match exactly (by helping narrow down the problem), and makes sure that both test output and examples are valid data of the purported format.
utAck, seems useful!
Yes, makes sense. I should have included something like this when I added the json tests.
I think #9023 (which outputs a contextual diff for failing test cases) should make it much easier to track down formatting errors, since it'll point you to the exact line that's different.
However, I think this is still useful for verifying that the output is indeed valid json.
Only nit is that compare_output() can raise errors which aren't caught anywhere and don't result in useful logging. For example, here's the logging if the output file is invalid json:
~/bitcoin/src$ test/bitcoin-util-test.py --srcdir='.'
Traceback (most recent call last):
File "test/bitcoin-util-test.py", line 32, in <module>
bctest.bctester(srcdir + "/test/data", "bitcoin-util-test.json", buildenv, verbose = verbose)
File "/home/vagrant/bitcoin/src/test/bctest.py", line 72, in bctester
bctest(testDir, testObj, buildenv.exeext)
File "/home/vagrant/bitcoin/src/test/bctest.py", line 50, in bctest
if not compare_output(outs[0], outputData, outputType):
File "/home/vagrant/bitcoin/src/test/bctest.py", line 14, in compare_output
match = (json.loads(a) == json.loads(b))
File "/usr/lib/python2.7/json/__init__.py", line 338, in loads
return _default_decoder.decode(s)
File "/usr/lib/python2.7/json/decoder.py", line 366, in decode
obj, end = self.raw_decode(s, idx=_w(s, 0).end())
File "/usr/lib/python2.7/json/decoder.py", line 382, in raw_decode
obj, end = self.scan_once(s, idx)
ValueError: Invalid control character at: line 3 column 14 (char 95)
It'd be nice to wrap the json calls in try/except, and maybe have the final else branch of compare_output() print an error and exit rather than raise an error.
Yea, that error 'Invalid control character at: line 3 column 14 (char 95)" is actually very good and specific. The only thing missing is context of what file it occurs in. Would make sense to print it with the filename.
I think #9023 (which outputs a contextual diff for failing test cases) should make it much easier to track down formatting errors, since it'll point you to the exact line that's different.
I too think #9023 is useful, and we should do that too, however this solves a different issue: say the JSON library is changed to pretty-print slightly differently. JSON itself is defined by a standard, but pretty-printing is not. This verifies separately that the functional part (the data) still matches. Once you now that, you can harmlessly copy the new formatting if you agree with it.
11 | +def compare_output(a, b, fmt): 12 | + '''Data-format dependent output comparison.''' 13 | + if fmt == 'json': # json: compare parsed data 14 | + match = (json.loads(a) == json.loads(b)) 15 | + elif fmt == 'hex': # hex: parse and compare binary data 16 | + match = (binascii.a2b_hex(a.rstrip()) == binascii.a2b_hex(b.rstrip()))
Why is this rstripped rather than stripped? Is there a case where you want to fail here if leading whitespace differs?
Leading whitespace is much harder to accidentally introduce than trailing whitespace (e.g. trailing spaces, newlines etc). But I'm fine with changing this to strip() instead.
Error parsing expected output blanktx.json as json: Expecting property name: line 4 column 18 (char 179)
Error parsing expected output blanktx.hex as hex: Non-hexadecimal digit found
Error parsing command output as hex: Non-hexadecimal digit found
This splits the output comparison for `bitcoin-tx` into two steps:
- First, check for data mismatch, parsing the data as json or hex
depending on the extension of the output file
- Then, check if the literal string matches
For either of these cases give a different error.
This prevents wild goose chases when e.g. a trailing space doesn't match
exactly, and makes sure that both test output and examples are valid
data of the purported format.
Looks good. Tested ACK
utACK 6c5cd9d