-
Notifications
You must be signed in to change notification settings - Fork 467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FAST payment channel tests #2319
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things to note about the way these tests are written at the moment.
- They use real nodes with real address, no more fixture / fake address 🎉
- Mining is done in the background. There are no calls to
mining once
.
I'm not sure if we want this to be a common / default pattern, or if we want to really have explicit control on exactly how many blocks are mines, who mines them, and when they are mined. I know some tests will need this, but I'm not sure if they all do.
Tests now run in ~30-35 seconds, prior tests were in the range of 12s, so this is a significant increase in time. I think this is largely due to the background mining. When I changed the mining time from 5s to 2s, the tests ran in ~16s. I ran the tests with a 1s block-time and experienced some flakes (the issue is that payment channels have requirements around the block-height for expiry).
This is possibly one reason to default to explicit mining.
a326294
to
b0f308b
Compare
I swapped background mining out for explicit mining. With a block time configured to 1s (same as TestDaemon), the FAST payment channel tests run in ~14 seconds. This is pretty good, and the additional time is from a bit more overhead in setting up nodes. I also tested with a millisecond block time (which I believe is used for the delay function), and have achieved test runs in the ~8 second range without issue. |
9aa9c58
to
900ae8d
Compare
ac06646
to
dcf5b35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a big improvement. I'm a little concerned we're losing coverage of the text encoding of command responses, but not enough to feel this shouldn't go through. I've requested some small changes that are mostly optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a lot of comments were adding while I was reviewing this, going to give this a second pass shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Travis. I haven't reviewed in detail since there are a bunch of changes suggested already that will make that review much easier. I do want to see it again before landing.
This is a biiig diff. Could you please factor out the renames and some of the basic FAST changes to a separate PR that can land first (a "prefactor") to reduce the noise on this one?
I have made mention elsewhere but to be super clear here, I don't think a regression to the range of 30s is acceptable for "normal" integration/daemon tests that developers are expected to run with any frequency. Background mining might be appropriate for some even higher level function tests, but I'd imagine these running only in CI by default, like other such tests. The small slow down of FAST with explicit block mining is probably worthwhile for the code sharing, especially since it looks likely we can improve that further with shorter block times and other optimisations. I think that the usability of tests and test-writing is a high value area for improvement over the next couple of months, and making tests fast is a huge part of that. |
PR replace by #2528 |
This is still a bit of a work in progress. There are some patterns that I think need to be worked out.
Also, currently tests written with FAST can flake due to the added local go-filecoin iptb plugin feature to copy the target binary (to ensure binary capability for all
go-filecoin
commands). This appears to be related to a bug being tracked in golang (golang/go#22315) due to an issue around file descriptors being held onto longer than needed inside of threads.A possible work around might be to use exec to run
cp
to ensure there are no open file descriptors being held by go threads.