Paths

Table of Contentst

Better error messages if bitcoind binary cannot be found
AbandonedPublic
Actions

Authored by schancel on Jun 28 2017, 07:07.

Details

Reviewers

deadalnix
sickpig
kyuupichan
awemany
freetrader

Group Reviewers

Restricted Project

Summary

Running some tests out-of-tree which pass a 'binary' argument
to start_node can result in the default 'bitcoind' binary
being unavailable. This can also happen if it is just deleted
and the user forgot to rebuild it.

This patch makes tests emits a friendlier message than the
huge "FileNotFound" traceback dump.

Now the test should report to the user:

Unable to locate bitcoind binary (./src/bitcoind).
Try specifying using --testbinary option, or set BITCOIND environment variable.

This offers two ways of passing the information about the correct
binary location.

A new framework self-test was added in bitcoind_config_problems.py,
to test the error messages related to bitcoind configuration problems.

Test Plan

cd build # out of tree
../configure # ... with whatever options you usually need
make check
rm src/bitcoind # remove the binary
../qa/rpc-tests/abc-p2p-fullblocktest.py # get the above error message
make # get the linked binary back
mv src/bitcoind src/mybitcoind # move it to another file
../qa/rpc-tests/abc-p2p-fullblocktest.py # get the above error message
export BITCOIND=$(pwd)/src/mybitcoind # point at renamed binary
../qa/rpc-tests/abc-p2p-fullblocktest.py # test runs ok
unset BITCOIND
../qa/rpc-tests/abc-p2p-fullblocktest.py --testbinary=$(pwd)/src/mybitcoind # test runs ok
sh ../qa/rpc-tests/test_framework/tests/run_self_tests.sh # fails because BITCOIND not set
export BITCOIND=$(pwd)/src/mybitcoind
sh ../qa/rpc-tests/test_framework/tests/run_self_tests.sh # all tests muss pass
unset BITCOIND
../qa/pull-tester/rpc-tests.py abc-p2p-fullblocktest # test fails with error msg on stderr
../qa/pull-tester/rpc-tests.py # full suite fails with similar errors
make # restore the default binary
../qa/pull-tester/rpc-tests.py abc-p2p-fullblocktest # single test runs ok
../qa/pull-tester/rpc-tests.py # full suite runs ok

For in-tree testing, just skip the 'cd build' and repeat the tests
removing the ../ prefix on relevant commands.

Diff Detail

Repository

rABC Bitcoin ABC

Branch

improve_out_of_tree_testing

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 443
Build 443: arc lint + arc unit

Event Timeline

freetrader created this revision.Jun 28 2017, 07:07

Harbormaster completed remote builds in B430: Diff 702.Jun 28 2017, 07:07

Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJun 28 2017, 07:07

No. If the argument passed down is wrong, you want to report it, not do wild guesses that may or may not be what the caller intended.

This revision now requires changes to proceed.Jun 28 2017, 07:19

In D282#4611, @deadalnix wrote:

No. If the argument passed down is wrong, you want to report it, not do wild guesses that may or may not be what the caller intended.

Agreed, but this may leave certain tests unable to run out of tree due to how they pass in binary without their callers properly setting SRCDIR.
I can live with reporting that.

Repurpose this diff to make it into better error handling for misconfiguration.

Harbormaster completed remote builds in B443: Diff 719.Jun 30 2017, 16:40

freetrader retitled this revision from Invoke bitcoind binary search if default testbinary cannot be found to Better error messages if bitcoind binary cannot be found.Jun 30 2017, 16:43

freetrader edited the summary of this revision. (Show Details)

freetrader edited the test plan for this revision. (Show Details)

If the problem is test not setting SRCDIR, then the best option is to use a default value for SRCDIR when it is not set, not making wild guesses.

deadalnix requested changes to this revision.Jul 3 2017, 15:02

This revision now requires changes to proceed.Jul 3 2017, 15:02

The test framework is using the default value of '.' for SRCDIR .

This works pretty well when SRCDIR is not set if the user is in one of the standard locations (top of tree, or in build/ folder within tree) .

I can certainly remove the locate_bitcoind_binary function and call if that's what you mean by 'wild guesses' .

@deadalnix : Can you please get specific about what you mean by 'wild guesses' in the current patch .

Returning to review, maybe someone else can answer my question?

The whole get_canonical_bitcoin_binary is an exercise in divination. This is improper for tests.

This diff seems to contain many unrelated changes. Error handling change, functionality change and change in how to locate the binary. Split it up.

qa/rpc-tests/test_framework/util.py
400	So binary can be an array now ? Why for ?
418	What's the point of catching and then rethrowing immediately ?
462	Catching exception based on error message seems fragile at best. I'm not aware of what's commonly done in python, but if this is considered good practice, this is scary.
475	Why does this needs to be in the finally block ? this only runs on error.
491	Initializing nodes to something sensible would be a more appropriate solution.

This revision now requires changes to proceed.Jul 4 2017, 23:04

freetrader added inline comments.Jul 5 2017, 10:09

qa/rpc-tests/test_framework/util.py
400	Certain args like extra_args and binary have traditionally been lists, and are indexed by i . This patch does not introduce that - it is used by a lot of tests.
418	This does seem futile.
462	No, we could certainly derive an UnableToLocateBitcoindError exception and use that instead.
491	Agreed, initializing 'nodes' to an empty list would be better.

The whole get_canonical_bitcoin_binary is an exercise in divination. This is improper for tests.

I don't agree with calling it divination.

It is working based on configuration from user (SRCDIR and BITCOIND) which have been in the test framework before.
Apart from that, it is just checking the sensible combination of these and the test executable (binary) supplied by the caller - or its default value if caller did not specify.

If this is improper, we should first specify what is proper. The way to do this is to either write up what you want to support by means of requirements, or give clear indication of which features of the configuration you want to remove.

qa/rpc-tests/test_framework/util.py
475	You're right, it doesn't need to be in a finally block.

Divination maybe is a strong term. But it does special stuff based on the name of the executable, and try to look for it in various places. It should only look for it in SRCDIR , and a sensible default should be provided for SRCDIR when it is not set.

OK, I will look at splitting this up into digestable separate diffs.

There is a traditional default value for SRCDIR that is still used by this patch - the current directory (.)

I'll examine what else SRCDIR is used for in the framework.
If there's no real need for both SRCDIR and BITCOIND, then maybe we should simplify to just having one.

It certainly makes it complicated to have two things which can then interact.