Venus: Move FAST retrieval test to functional-tests, delete shell test
Description
I believe the test written in tools/fast/tests/retrieval_test.go covers the same surface area as the shell script we current have. I think FAST is at a place we can remove the shell script and helper library.
Acceptance criteria
- Remove current bash retrieval script
- Move FAST retrieval test into
functional-testsdirectory
Risks + pitfalls
- Risk of loosing test coverage if the tests differ wild in execution. We should ensure the functionality is the same prior to doing this work.
Where to begin
travisperson
All 9 comments
Go go go!
anorth
on 12 Apr 2019
@travisperson I sensed some resistance when removing the shell test came up in discussion. Can we address that here? In particular it seems that a few people use the functional test frequently as a sanity check. While in the long term I think that's an unhealthy habit (unit and integration tests should provide sufficient confidence), we should retain support for that use case.
I expect that the issue is that people don't know how to run the FAST version or interpret its output. Will it be as simple as go test -functional? Can you speak to how output can be inspected in the FAST version vs the shell version?
If anyone has other objections, please raise them here now.
anorth
on 17 Apr 2019
I use this retrieval test when developing locally. I also use this retrieval test to validate downloaded go-filecoin binaries after a release (local network), and I use it (with some modifications) to test our devnets too.
I'd be quite bummed out to lose it, but I am aware that the software that makes up the "retrieval test" has a carrying cost. I've seen a very high ROI on that software to-date (which is to say: it's caught a lot of bugs during development and hasn't required a lot of maintenance).
As a compromise: I can agree to try supplant my usage of the retrieval script for the next couple of weeks, replacing it with the FAST retrieval test. If after a couple of weeks I haven't found any cause for complaint (e.g. it's not difficult to see the log streams from the various go-filecoin nodes being tested, errors aren't swallowed, etc.) then I will support your efforts to delete my beloved retrieval test.
How's that sound?
laser
on 17 Apr 2019
cc @acruikshank @rosalinekarr @rkowalick @shannonwells ^
laser
on 17 Apr 2019
@travisperson, does the FAST retrieval test cover updating a peer id? I can't find any part of it that does, and I know that's one thing the shell tests covers which has saved from merging a bug at least once.
rosalinekarr
on 17 Apr 2019
Also, fwiw, FAST still has some outstanding issues regarding properly surfacing errors upon failure which the shell test currently handles better.
rosalinekarr
on 17 Apr 2019
My first step in reproducing an issue with filecoin is to comment out or modify certain parts of the retrieval shell script and run a debugger on a filecoin daemon.
The script will be sorely missed, as I have never found a simpler way to reproduce issues with our software.
I also feel bash is appropriate for a daemon and client that are shell based.
lkowalick
on 17 Apr 2019
From my standpoint, the primary value of the retrieval script is as template for ad hoc automation of complicated interactions. I'm with @anorth that the fact that this is significantly easier in bash is a bug in our integration testing setup, not a feature of bash scripting.
There are a lot of benefits to FAST. One of its original design criteria was that it should be as easy to implement and use as bash scripting. This is where we get to find out how much progress we've made towards that goal.
My desire is to implement the retrieval test in FAST and leave the retrieval script in the repo (maybe marked as deprecated). We'll then be able to use both and compare. If we find we just can't live without the retrieval script, we should learn from that and fix FAST. Otherwise we delete it later.
acruikshank
on 18 Apr 2019
I've added some documentation in PR #2610 which covers some of the debugging abilities of FAST
travisperson
on 24 Apr 2019
Related issues
phritz
路
5Comments
ZenGround0
路
6Comments
shannonwells
路
5Comments
laser
路
4Comments
phritz
路
5Comments
Most helpful comment
I use this retrieval test when developing locally. I also use this retrieval test to validate downloaded go-filecoin binaries after a release (local network), and I use it (with some modifications) to test our devnets too.
I'd be quite bummed out to lose it, but I am aware that the software that makes up the "retrieval test" has a carrying cost. I've seen a very high ROI on that software to-date (which is to say: it's caught a lot of bugs during development and hasn't required a lot of maintenance).
As a compromise: I can agree to try supplant my usage of the retrieval script for the next couple of weeks, replacing it with the FAST retrieval test. If after a couple of weeks I haven't found any cause for complaint (e.g. it's not difficult to see the log streams from the various go-filecoin nodes being tested, errors aren't swallowed, etc.) then I will support your efforts to delete my beloved retrieval test.
How's that sound?