pytest: Add grpc based CLN client to test grpc coverage #5362

cdecker · 2022-06-30T10:27:46Z

The goal here is to extend test coverage to the grpc interface. We can do so by adding a new LightningGrpc class and swapping out the LightningRpc class if configured to do so. Swapping out then means that instead of talking directly to the JSON-RPC:

CLN -- JSON-RPC -- LightningRpc -- pytest
\_____CLN_____/    \_______pytest_______/

we now take a bit of a detour:

CLN -- JSON-RPC -- cln-rpc -- rpc2grpc converters -- grpc interface -- python grpc client -- python grpc2json converter -- pytest
\_____CLN_____/    \___________cln-grpc-plugin____________________/    \__________________________pytest________________________/

It's a bit roundabout, but it allows us to test all of the components along the way without having to change the tests themselves, and after all they should expose the same functionality.

To opt-in to the grpc testing for now you have to set an environment variable CLN_TEST_GRPC=1, and the LightningNode will swap out the RPC implementation.

Caveat: the conversion is sometimes lossy, i.e., some fields aren't converted (yet), and some RPC methods aren't implemented (and some may never be). Hence some tests will simply not be possible to run at the moment, but I am working on extending coverage, both method-wise as well as field-wise, just be warned that some

rustyrussell

One cut & paste bug, and some questions about our lack of test coverage which allowed these schema issues to slip through CI!

But other than that, looks good!

rustyrussell · 2022-07-17T02:19:02Z

doc/schemas/stop.request.json

+  "required": [
+    "psbt"
+  ],


I think this is wrong!

Also, we clearly don't test this properly. Why did it pass CI?

Curious indeed, so I went digging a bit:

lightning/contrib/pyln-testing/pyln/testing/utils.py

Lines 655 to 658 in 6204d70

# We only check payloads which are dicts, which is what we

# usually use: there are some cases which tests [] params,

# which we ignore.

if schemas and schemas[0] and isinstance(payload, dict) and self.check_request_schemas:

The payload is None in this case, hence falsifying the third clause, and we skip schema validation. Even if we did check the schema, we'd run into this:

lightning/contrib/pyln-testing/pyln/testing/utils.py

Lines 856 to 861 in 6204d70

# Tell the daemon to stop

try:

# May fail if the process already died

self.rpc.stop()

except Exception:

pass

So we might want to relax that if statement and only catch a specific error type instead.

Will fix up the schema though ^^

rustyrussell · 2022-07-17T02:22:01Z

doc/schemas/fundchannel.request.json

+    },
+    "mindepth": {
+      "description": "Number of confirmations required before we consider the channel active",
+      "type": "u32"
    }


THis implies we never tested this parameter (or at least, never as a named parameter!).

The issue here is that the fundchannel.request.json file is new from this PR (the fundchannel.schema.json is the one that was tested).

When I backfilled it early in the PR we didn't have the zeroconf PR merged yet, hence why we needed to add it only later. And indeed CI and make pytest surfaced this lack of parameter in the first version of the JSON schema, hence why I added it here.

TL;DR: Things are working as expected, and we even verified that a missing param does trigger CI warnings.

cdecker · 2022-07-19T07:46:33Z

Rebased and removed copy-pasta.

Addressed

We were overriding the name right when loading, which is bad since in some languages we use the method name as tag in the requests, thus renaming causes us to call something that isn't defined. Changelog-Fixed: cln-rpc: Fixed a naming mismatch for `ConnectPeer` causing `connectpeer` to be called on the JSON-RPC

These may eventually end up in pyln-client, as they allow talking to the GRPC interface exposed by cln-grpc, however for now they are used for testing only. Once we have sufficient API and test coverage we can move them and leave imports in their place.

To test the grpc interface we'll want to emulate the JSON-RPC interface as best we can, hence when talking to the grpc interface we want to convert back into a parsed JSON format as LightningRpc would have returned it. This is just the simplest way of closing the loop here: ``` pyln-testing --grpc-> cln-grpc --grpc2json ^ | | v | JSON-RPC | | TEST v ^ CLN | | | v pyln-testing <-grpc2py-- cln-grpc <- json2grpc ```

This allows us to re-use existing tests (assuming the call and fields are covered by `cln-rpc` and `cln-grpc`) to test the full roundtrip from test over the grpc interface to the json-rpc interface and back again. You can switch to the grpc interface by setting the `CLN_TEST_GRPC` environment variable to 1, but for now only very few shims are implemented (due to the non-generated nature of LightningRpc).

The CLN API is rather strict about the fact that we should skip providing a field whenever it is empty. Checking for `is_none` would still include empty arrays. Changelog-Fixed cln-rpc: Optional empty arrays will no longer be serialized in requests

This is usually a signal that lightningd is shutting down, so notify any instance that is waiting on `plugin.join()`. Changelog-Fixed: cln-plugin: Fixed an issue where plugins would hang indefinitely despite `lightningd` closing the connection

We'll need this when testing the grpc interface in pyln-testing, otherwise tests just slowly die and wither.

These are the most used methods in tests, so we can start getting our test coverage up.

It's being skipped in grpc so we don't have it later anyway.

This was likely missed during the zeroconf PR. Changelog-None

rustyrussell · 2022-07-20T20:12:01Z

Ack 0741499

cdecker force-pushed the cln-grpc-testing branch 4 times, most recently from 7c172ce to f990dfd Compare July 4, 2022 11:48

niftynei added this to the v22.10 milestone Jul 11, 2022

cdecker mentioned this pull request Jul 11, 2022

cln: Add more methods to msggen, cln-rpc, and cln-grpc #5311

Closed

cdecker force-pushed the cln-grpc-testing branch 6 times, most recently from 1aa273f to 7e93891 Compare July 13, 2022 15:48

niftynei modified the milestones: v22.10, v0.12 Jul 13, 2022

cdecker marked this pull request as ready for review July 15, 2022 06:57

rustyrussell previously requested changes Jul 17, 2022

View reviewed changes

cdecker force-pushed the cln-grpc-testing branch from 4c0ba71 to a34a595 Compare July 19, 2022 07:45

cdecker requested a review from rustyrussell July 19, 2022 07:46

cdecker force-pushed the cln-grpc-testing branch from a34a595 to 40b701d Compare July 19, 2022 10:05

cdecker force-pushed the cln-grpc-testing branch 3 times, most recently from c19dde2 to d09faea Compare July 19, 2022 19:59

cdecker mentioned this pull request Jul 20, 2022

openingd: Allow users to set the enforced reserve for their counterparty (zeroreserve) #5315

Merged

1 task

cdecker added 5 commits July 20, 2022 16:18

msggen: Add fundchannel request

4724cde

cdecker added 13 commits July 20, 2022 16:18

msggen: Add stop method to generators

39891ac

We'll need this when testing the grpc interface in pyln-testing, otherwise tests just slowly die and wither.

schema: minconf should be an integer (u32) not a float (number)

d60c264

pyln-testing: Add a couple of methods used in tests

6391cc7

These are the most used methods in tests, so we can start getting our test coverage up.

cln-rpc: Add mindepth after rebase on master

48688ab

msggen: Ignore state_changes in grpc2py

0270d7f

It's being skipped in grpc so we don't have it later anyway.

pyln-testing: Add invoice RPC method

1b9d683

cln-plugin: Add metadata required by crates.io

80abccf

cln-plugin: Fix plugin dependencies

7ec272d

pyln: Ignore generated files when linting

7fb54d8

schema: Add missing mindepth argument to fundchannel

487c8b0

This was likely missed during the zeroconf PR. Changelog-None

pyln-testing: Add listinvoices to grpc shim

0741499

cdecker force-pushed the cln-grpc-testing branch from d09faea to 0741499 Compare July 20, 2022 14:23

rustyrussell merged commit 76d0548 into ElementsProject:master Jul 21, 2022

cdecker deleted the cln-grpc-testing branch July 20, 2023 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pytest: Add grpc based CLN client to test grpc coverage #5362

pytest: Add grpc based CLN client to test grpc coverage #5362

cdecker commented Jun 30, 2022

rustyrussell left a comment

rustyrussell Jul 17, 2022

rustyrussell Jul 17, 2022

cdecker Jul 17, 2022

cdecker Jul 17, 2022

rustyrussell Jul 17, 2022

cdecker Jul 17, 2022

cdecker commented Jul 19, 2022

rustyrussell commented Jul 20, 2022

	# We only check payloads which are dicts, which is what we
	# usually use: there are some cases which tests [] params,
	# which we ignore.
	if schemas and schemas[0] and isinstance(payload, dict) and self.check_request_schemas:

	# Tell the daemon to stop
	try:
	# May fail if the process already died
	self.rpc.stop()
	except Exception:
	pass

pytest: Add grpc based CLN client to test grpc coverage #5362

pytest: Add grpc based CLN client to test grpc coverage #5362

Conversation

cdecker commented Jun 30, 2022

rustyrussell left a comment

Choose a reason for hiding this comment

rustyrussell Jul 17, 2022

Choose a reason for hiding this comment

rustyrussell Jul 17, 2022

Choose a reason for hiding this comment

cdecker Jul 17, 2022

Choose a reason for hiding this comment

cdecker Jul 17, 2022

Choose a reason for hiding this comment

rustyrussell Jul 17, 2022

Choose a reason for hiding this comment

cdecker Jul 17, 2022

Choose a reason for hiding this comment

cdecker commented Jul 19, 2022

rustyrussell commented Jul 20, 2022