Connor/cli smoke test #1196
Connor/cli smoke test #1196
Conversation
connormullett
requested review from
creativcoder,
cryptoquick and
ec2
as code owners
docs/SMOKE_TESTING.md
Outdated
| ## Prerequisites | ||
| In order to smoke test the RPC API endpoints, you need to meet 2 pre-requisites. | ||
| * A forest node should be running locally on port `1234` | ||
| * `FULLNODE_API_INFO` needs to be set as an environment variable. This can be done | ||
| as a prefix to the actual command, set with a script that is sourced (linux), | ||
| or set via the command line. If you don't know the multiaddr of the node, you can | ||
| use `forest auth api-info` to get the key-value pair needed. |
There was a problem hiding this comment.
What do you think of creating a temporary config in /tmp and then use the new FOREST_CONFIG env var to disable keystore encryption? Then all these commands should work automatically, no FULLNODE_API_INFO variable needed. The new changes in the last api-info PR made it so the keystore is unlocked without necessarily needing that anymore, unless you're testing with different permissions levels.
There was a problem hiding this comment.
I like it, let me see if I can wrangle something like that together. Currently permissions are not being tested. These smoke tests are just testing if the endpoints can be hit and a response is returned.
There was a problem hiding this comment.
Cool! And yeah, it's fine to not test permissions for a first stab at this.
There was a problem hiding this comment.
Smoke tests should not test permissions. If we want to test permissions we should set up some auth testing which would be similar to integration tests
There was a problem hiding this comment.
Honestly, I'm not sure there's a decent way to do a comprehensive evaluation beyond a basic validation of the auth mechanism itself since the Lotus OpenRPC schema doesn't specify permissions.
I do think auth levels should be verified against at least one endpoint through smoke tests, but I'm fine with you implementing that later or some other way.
There was a problem hiding this comment.
Implemented the requested changes. Should be ready now
There was a problem hiding this comment.
Cool, I'll give it another look
There was a problem hiding this comment.
@connormullett Also, can you update the docs for this? I don't think we need the env variable now, right?
There was a problem hiding this comment.
You are right, we don't need that anymore. Good catch!
node/rpc/src/lib.rs
Outdated
| @@ -128,7 +128,6 @@ where | |||
| .with_method(STATE_REPLAY, state_replay::<DB, B>) | |||
| .with_method(STATE_NETWORK_NAME, state_network_name::<DB, B>) | |||
| .with_method(STATE_NETWORK_VERSION, state_get_network_version::<DB, B>) | |||
| .with_method(STATE_REPLAY, state_replay::<DB, B>) | |||
There was a problem hiding this comment.
Umm, IDK? Might have misclicked something
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
#1040
Other information and links