Skip to content

fix: using runner_features and stash_field_from_event inside tests #129

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

Merged
merged 1 commit into from
Dec 21, 2024

Conversation

Psycho-Pirate
Copy link
Contributor

Tests are updated to use runner_features and stash_field_from_event, this allows the tests to support multiple lightning implementations at the same time.

@Psycho-Pirate Psycho-Pirate changed the title using runner_features and stash_field_from_event inside tests fix: using runner_features and stash_field_from_event inside tests Dec 7, 2024
@vincenzopalazzo

This comment was marked as off-topic.

@vincenzopalazzo
Copy link
Collaborator

Thanks @Psycho-Pirate for this but I need to ask why we need this change, and if you can explain it inside the commit body.

IIRC this change is needed when different implementations require different features bit by default. I am right about this?

If yes can you add a detailed description inside it? Can everyone who reads your code use the commit as separate documentation?

@Psycho-Pirate
Copy link
Contributor Author

Sorry for delay, before I digg inside the review, can you please fix the Ci?

I think this might be a core-lightning bug because it is passing with LDK

runner_features provides the features required by an implementation to Lnprototest.
stash_field_from_event allows retrieval of information from the message that was
just decoded within the `ExpectMsg` event.

Using these methods will make Lnprototest more generalised and compatible with other lightning implementations.
@Psycho-Pirate
Copy link
Contributor Author

Thanks @Psycho-Pirate for this but I need to ask why we need this change, and if you can explain it inside the commit body.

IIRC this change is needed when different implementations require different features bit by default. I am right about this?

If yes can you add a detailed description inside it? Can everyone who reads your code use the commit as separate documentation?

Added a descriptive commit message

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e37306f

Thanks!

Comment on lines +17 to +24
import pytest
import time
from lnprototest.utils import utxo, tx_spendable


def test_gossip_timestamp_filter(runner: Runner) -> None:
if runner.has_option("option_gossip_queries") is None:
unittest.SkipTest("Needs option_gossip_queries")
pytest.skip("Needs option_gossip_queries")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good change, but usually need a separate commit

@@ -165,7 +165,7 @@ def update_checksums(update1: Optional[Message], update2: Optional[Message]) ->

def test_query_channel_range(runner: Runner) -> None:
if runner.has_option("option_gossip_queries") is None:
unittest.SkipTest("Needs option_gossip_queries")
pytest.skip("Needs option_gossip_queries")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@vincenzopalazzo vincenzopalazzo merged commit ed8a06d into rustyrussell:master Dec 21, 2024
3 of 4 checks passed
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.

2 participants