functional tests: wait for sync and wait for height helpers, expose timeouts to environment variables, timeouts for stallment to avoid misleading breaks

[jaoleal]

Description and Notes

Here i present some general DX improvements for the functional tests, with simple changes on how we deal with timeouts.

The tests are not bounded by CPU speed but bounded by IO heavy usage, thats why the stale check helps to provide preciser timeouts.

Heres a breakdown of the impact of the changes on my machine:

- master, no workers definition (defaults to 4) - 28 passed in 252.09s (0:04:12).

- master, auto workers definition (14) - 3 failed, 22 passed in 90.21s (0:01:30). 
(failure because of IO read reached timeout).
 test_get_ping, test_add_node_v2, getconnectioncount failed

- this branch, no workers definition (default to 4) - 28 passed in 257.95s (0:04:17) 

- this branch, auto workers definition (14) - 28 passed in 143.31s (0:02:23)

With this we can add the -n auto flag to CI and see if anything breaks, Ill do that after the checks for this PR are completed and ill add them here.

CI runs

master, no workers definition ( defaults to 4 ) - 28 passed in 70.67s (0:01:10)  

this branch, no workers definition ( defaults to  4 ) - 28 passed in 73.71s (0:01:13)

this branch, workers on auto (still 4) - 28 passed in 142.72s (0:02:22)

Yeah, CI is a lost cause for performance... luisschwab When you will give us a CI runner ?

How to verify the changes you have done?

Run the prepare, the general just test-functional does not accept args and will run prepare and build binaries whitout need:

just test-functional-prepare

In master and checked in this branch:

just test-functional-run '-n auto'

and youll see that tests will break on master but not on this branch.

Speculation

The major bound still IO, this is proven by the tests breaking because of READ timeouts. Perhaps we could make tests to run totally on ram ? Can we cache datadirs ? Can we avoid to write in disk on test environments ? The logs are being more usefull to debug running phase than datadirs itself

The tests got faster on more workers not necessarily because of CPU working more but because more tests were running at the same time, IO usage spiked even more in this case.

Also, this show us that we can optimize tests even more... Why ? you ask me.

Its obvious! Faster and well engineered test framework will allow us to run those tests on more environments, being able to verify floresta functionality in more cases and even add more extensive heavy load tests whitout bothering about the execution phase.

[Micah-Shallom]

NIT: tested locally with this snippet:

@pytest.mark.rpc
def test_rpc_after_daemon_killed(florestad_node):
    florestad = florestad_node
    florestad.rpc.get_blockchain_info()           # alive
    florestad.daemon.process.kill()
    florestad.daemon.process.wait()
    start = time.time()
    try:
        florestad.rpc.get_blockchain_info()
    except Exception:
        elapsed = time.time() - start
        pytest.fail(f"RPC took {elapsed:.2f}s to fail")

ran on both master and on this PR

an RPC call after the daemon is killed fails in 0.00s on master but on this branch it takes 60.27s before failing which is exactly the REQUEST_STALE_TIMEOUT default

the retry on ConnectionError makes sense for the startup race under -n auto....but it also fires after a successful RPC, so a daemon crash mid-test eats the full 60s before failing......

could the retry distinguish "haven't connected yet" from "was connected, now isn't"?

orrrrr Is the 60s wait time not much of a big deal??

[jaoleal]

Nice catch!

orrrrr Is the 60s wait time not much of a big deal??

No, it is, were trying to save time here, the intent for those timeouts is to help slow runners.

I think its possible to know wheter a node died or is just non-responsive. Another nice thing for this pr is the centralization for those requests, so this should be straightforward.

[Micah-Shallom]

alright....make sense

[Davidson-Souza]

For the "wait fro sync" part, I think core have a RPC for that. It basically hangs until the daemon reaches a certain height. We could do that as well

[moisesPompilio]

For the "wait fro sync" part, I think core have a RPC for that. It basically hangs until the daemon reaches a certain height. We could do that as well

This is an excellent idea, but the error that will occur if the sync hasn’t finished will be a request timeout. In that case, we just need to map that error to indicate the correct cause.

Currently, the maximum time we wait for a request to complete is 15 seconds. For chain sync, we may need to increase that limit, because on weaker computers 15 seconds might not be enough. I think we can safely raise it to somewhere between 45 and 60 seconds.

[moisesPompilio]

Using while true is not a good idea. The ideal approach is to handle the timeout case here, because inside the while, if that happens, it can exit by returning immediately. If it keeps going after the timeout, it ends up causing an error.

In this PR here: #897 I added a wait_until helper that already does this automatically, so it makes things easier.

[jaoleal]

For the "wait fro sync" part, I think core have a RPC for that. It basically hangs until the daemon reaches a certain height. We could do that as well

Yeah, this is really interesting... Moves the timeout to the node side and perhaps will get rid of some python overhead.

But for this Pr I insist on the current python-centric approach, I think its better to leave this specific rpc to work in the future and bring a more elegant solution for it.

[moisesPompilio]

Move this to util.py