Skip to content
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

[WIP] lnprototest refactoring #95

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

vincenzopalazzo
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo commented Jul 3, 2023

We are embarking on an experimental journey with lnprototest.

Presently, lnprototest exhibits a certain level of intricacy in our test-writing approach.

Thus, this pull request (PR) introduces an idea that emerged from discussions on the core lightning Discord.

The essence of this idea involves transforming the runner into a stateless entity while endowing the connection with statefulness. By adopting this approach, we can embrace a more procedural manner of crafting tests (idea stoled from the PoC of Rusty https://github.com/rustyrussell/lnprotest).

Allow me to illustrate with an example:

def test_v2_init_is_first_msg(runner: Runner, namespaceoverride: Any) -> None:
    """Tests workflow
    runner --- connect ---> ln node
    runner <-- init ------ ln node
    """
    runner.start()

    conn1 = runner.connect(None, connprivkey="03")
    init_msg = conn1.recv_msg()
    assert (
        init_msg.messagetype.number == 16
    ), f"received not an init msg but: {init_msg.to_str()}"
    conn1.send_msg("init", globalfeatures="", features="")
    runner.stop()

Subsequently, we should be able to circumvent code duplication and compose code in a maintainable manner. To achieve this, we are adopting the framework concept proposed by Christian, available at: https://github.com/cdecker/lnpt.

@lnpt.step()
def connect(c):
    # Open a connection

@lnpt.step(parents=[connect])
def init(c):
    # Send and receive init

Furthermore, in conclusion, we can package all these developments into a library, eliminating the need to include one of the lnprototest repositories. This approach empowers each implementation to construct its own tests, a capability that already exists at present.

For in-depth discussions, please refer to: #105.

Note: I am intending to maintain backward compatibility with the old version of lnprototest initially, but after a year, I plan to discontinue support for it. The eventual removal of the old code is anticipated within two years or sooner.

vincenzopalazzo added a commit that referenced this pull request Jul 15, 2023
This commit addresses an issue with the error checking
mechanism. Currently, the `ExpectError` function only verifies
the occurrence of an error during the connection, without considering
the specific error message expected.

The upcoming changes in [1] will simplify lnprototest, but until then,
this fix provides a straightforward and effective solution for
checking error messages.

[1] #95

Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo added a commit that referenced this pull request Jul 15, 2023
This commit addresses an issue with the error checking
mechanism. Currently, the `ExpectError` function only verifies
the occurrence of an error during the connection, without considering
the specific error message expected.

The upcoming changes in [1] will simplify lnprototest, but until then,
this fix provides a straightforward and effective solution for
checking error messages.

[1] #95

Signed-off-by: Vincenzo Palazzo <[email protected]>
@vincenzopalazzo vincenzopalazzo added this to the lnprototest 0.1.0 milestone Jul 15, 2023
vincenzopalazzo added a commit that referenced this pull request Jul 15, 2023
This commit addresses an issue with the error checking
mechanism. Currently, the `ExpectError` function only verifies
the occurrence of an error during the connection, without considering
the specific error message expected.

The upcoming changes in [1] will simplify lnprototest, but until then,
this fix provides a straightforward and effective solution for
checking error messages.

[1] #95

Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo added a commit that referenced this pull request Jul 15, 2023
This commit addresses an issue with the error checking
mechanism. Currently, the `ExpectError` function only verifies
the occurrence of an error during the connection, without considering
the specific error message expected.

The upcoming changes in [1] will simplify lnprototest, but until then,
this fix provides a straightforward and effective solution for
checking error messages.

[1] #95

Signed-off-by: Vincenzo Palazzo <[email protected]>
There is no reason to keep a generic interface in the
runner and provide the implementation in the core lightning runner
if the only way to establish a connection with the node is through
the pyln.proto package.

This commit moves the CLightningConn inside the runner interface
to allow everyone access to the default implementation.

Signed-off-by: Vincenzo Palazzo <[email protected]>
This commit marks the first step in the lnprototest
refactoring process, aimed at writing more readable
tests with the runner.

With this commit, it becomes possible to write tests
similar to the following example.

```
def test_v2_init_is_first_msg(runner: Runner, namespaceoverride: Any) -> None:
    """Tests workflow

    runner --- connect ---> ln node
    runner <-- init ------ ln node
    """
    runner.start()

    runner.connect(None, connprivkey="03")
    init_msg = runner.recv_msg()
    assert (
        init_msg.messagetype.number == 16
    ), f"received not an init msg but: {init_msg.to_str()}"

    runner.stop()
```

Signed-off-by: Vincenzo Palazzo <[email protected]>
This commit introduce the ability to stash information inside
the connection.

This function allow lnprototest to build connection that can
keep state by connection.

Signed-off-by: Vincenzo Palazzo <[email protected]>
This commit remove the dummy runner because it is starting to be painful
to keep supporting it with the addition of feature.

For historical reason I think the dummy runner was used to check the
logic of the lnprototest, but with the refactoring that we are doing.

Signed-off-by: Vincenzo Palazzo <[email protected]>
…nection

This commit enable the connection to send a message through the wire in
an ergonomic way.

This feature is a basic blocks for the lnprototest refactoring that
allow to semplify how to write test with lnprototest in the future
by keeping the state with the peer by connection and keep inside
the runner just the necessary logic to interact with the node.

Signed-off-by: Vincenzo Palazzo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant