pytest: Enable parallel test execution and start migration to modern fixtures#1460
Conversation
|
Just as a side note: I did not enable parallel test execution on Travis yet, since it was exacerbating the flakiness of some of the tests, but the builder already has all the depencies and with |
cdecker
force-pushed
the
pytest
branch
2 times, most recently
from
7571f6a to
3b33a5c
Compare
ZmnSCPxj
left a comment
ZmnSCPxj
left a comment
There was a problem hiding this comment.
Looks fine-ish to me. But, I am not Python user. So better to get someone else review. And, many question.
| # Force l3 to give its address. | ||
| l3port = self.node_factory.get_next_port() | ||
| l3 = self.node_factory.get_node(options={"ipaddr": "127.0.0.1:{}".format(l3port)}) | ||
| l3 = self.node_factory.get_node(options={"ipaddr": "127.0.0.1"}) |
There was a problem hiding this comment.
Is port not needed here? The reason why ipaddr is specified is to make l3 gossip its own address and port correctly, else l1 will be unable to look up l3 via gossip.
#1450 makes l3 gossip correctly (currently we only gossip --ipaddr or what is auto-detected as our address) so the option could be removed there.
There was a problem hiding this comment.
Hmm the CI seems to work, so I guess it does work? Hmm.
| if not random_hsm: | ||
| self.opts['dev-hsm-seed'] = binascii.hexlify(seed).decode('ascii') | ||
| self.prefix = 'lightningd(%d)' % (port) | ||
| self.prefix = 'lightningd-%d' % (node_id) |
There was a problem hiding this comment.
If two tests run in parallel, would they get separate node_factory? We instantiate a new factory at setUp. Sorry, but I am unfamiliar with the Python test environment (and Python in general); can tests in a single test class run in parallel or do they have to be separate standalone functions like in test_gossip.py in order to run in parallel?
|
I have another patch which guesses ports and restarts if it guesses wrong. But we can put that in after, to remove ephemeral ports dependency. |
|
Also, I hate this. Make already handles parallelism, so doing it again in pytest is redundant (and wrong, though it works in this case since make ends up just running pytest after everything else is finished). However, running individual tests via make gives v ugly output, so I had to redirect it all and that's also ugly. End "Old man yells at cloud" :) |
Slowly moving towards a more pythonic approach to the testing code. Signed-off-by: Christian Decker <decker.christian@gmail.com>
Especially with valgrind this allows us to safe quite some time on multi-core machines since startup is about the most expensive operation in the tests. In a simple test spinning up 3 nodes this gave me about 25% - 30% test time reduction. The effects will be smaller for single core machines, hoping Travis handles these gracefully. Signed-off-by: Christian Decker <decker.christian@gmail.com>
We never really look at the output, and it's rather noisy, so we just stop writing to the log. Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This is the first example of the py.test style fixtures which should allow us to write much cleaner and nicer tests. Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
…l testing Adds a new dependency, but totally worth it :-) Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We add an attempt number to the test directory to improve the test-isolation and allow for multiple reruns of the same test, without re-using any of the lightning-dirs or bitcoin-datadirs. Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This is mainly just a stopgap solution until we get to stabilize the test. Signed-off-by: Christian Decker <decker.christian@gmail.com>
This used to be the port, but since we no longer have fixed ports, and we start them in random order we can't easily distinguish them by the port anymore. Just use a numeric ID that matches their lightning-dirs. Signed-off-by: Christian Decker <decker.christian@gmail.com>
|
Rebased, will apply once CI happy... |
|
Had to restart a couple of tests, will submit a patch for one of them which I think I understand how it ran too slow... |
|
Ack 07f2e7d |
3 participants
I got a bit sidetracked while implementing the
local_channel_updatespatch (that's going to be my next PR). This PR modernizes the integration test environment using the pytest style fixtures, flattening the test structure (functions instead of class methods) and begins the migrating the tests into smaller, better structured test files. The goal is to split the hugetest_lightningdinto smaller, topic based test files that can be selectively executed for changes, e.g., executingtest_gossip.pyif we changed gossip and don't expect failures in other parts of the suite (CI will still catch those).One major change is that we now have the tests isolated into their own directory and selecting random ports for all daemons, which means that we can run the tests in parallel with
pytest-xdist:Which should speed up the change-test-fix cycle alot. One downside is that we add a few required dependencies:
pytestandephemeral-port-reserve, and since we're at it we also addflaky. These can be easily installed viapip3 install -r tests/requirements.txtand I strongly suggest using virtualenvs for these anyway.flakynow also works as a decorator, it'll just rerun the test once (and really produce an independent result thanks to the directory isolation). This is more of a stop-gap solution to be able to work with the CI, and should not free us from actually stabilizing the test though 😉