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

Add VSS Store CI Integ Test #161

Merged
merged 1 commit into from
Dec 12, 2023
Merged

Add VSS Store CI Integ Test #161

merged 1 commit into from
Dec 12, 2023

Conversation

G8XSU
Copy link
Contributor

@G8XSU G8XSU commented Aug 31, 2023

  • Pulls and runs vss-server
  • Runs kvstore testcases against local hosted instance of vss-server.

Depends on #158

@tnull
Copy link
Collaborator

tnull commented Sep 11, 2023

Thanks! Do we see a way to integrate it more closely? In particular, we should run channel_full_cycle against the VssClient backend and see that it works. Note that to that end I'll introduce a TestSyncStore that wraps all provided backends and asserts they behave identically (76d2f03). This is currently part of #159, but will be included as part of the upgrade to 0.0.117. We'll also want to add VssStore as part of that and run tests against all of these backends to be sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that we'll want to run the usual tests against this backend, so while it might be a bit cleaner to have a separate CI worflow, we'll probably need to do this setup as part of our 'regular' CI run, possibly as part of a vss feature flag for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can do this setup in separate workflow and run all tests against it.

I didn't want to bloat single ci workflow so much and wanted to maintain some separation of concerns regarding vss-integration in particular .

@G8XSU
Copy link
Contributor Author

G8XSU commented Sep 15, 2023

In particular, we should run channel_full_cycle

Yes, once we have a public api to build with vss, will add that.

@tnull
Copy link
Collaborator

tnull commented Sep 18, 2023

In particular, we should run channel_full_cycle

Yes, once we have a public api to build with vss, will add that.

There is no reason to wait for the public API, as we can just use build_with_store.

@G8XSU
Copy link
Contributor Author

G8XSU commented Sep 26, 2023

In particular, we should run channel_full_cycle

That test is now part of #158 , so that it is tested as part of this same ci

Rebased on #158 and #151

@G8XSU
Copy link
Contributor Author

G8XSU commented Sep 28, 2023

Depends on lightningdevkit/vss-server#25
to fix CI failure, rest should be good

@tnull
Copy link
Collaborator

tnull commented Nov 7, 2023

This needs a rebase now that #158 landed :)

@G8XSU
Copy link
Contributor Author

G8XSU commented Nov 8, 2023

Rebased on #158

@tnull
Copy link
Collaborator

tnull commented Nov 9, 2023

Rebased on #158

The integration test fails due to:

thread 'test::functional_tests::channel_full_cycle_with_vss_store' panicked at src/test/functional_tests.rs:31:92:
called `Result::unwrap()` on an `Err` value: ReadFailed

I.e., on build_with_vss_store:

https://github.com/G8XSU/ldk-node/blob/c1a26e51fb9f122e411f51384d345c0686b8ca06/src/test/functional_tests.rs#L31

@G8XSU
Copy link
Contributor Author

G8XSU commented Nov 14, 2023

I think this will be fixed once latest vss-client is published(since it depends on some changes in that), i will verify it once.

@G8XSU
Copy link
Contributor Author

G8XSU commented Dec 1, 2023

Depends on #213 for now.

@tnull
Copy link
Collaborator

tnull commented Dec 4, 2023

Depends on #213 for now.

Needs a rebase now that #213 has been merged.

In this, we spin up a local vss-server instance using tomcat,
and run vss related integration tests in ldk-node.
@G8XSU
Copy link
Contributor Author

G8XSU commented Dec 12, 2023

@tnull rebased and that error is fixed now.

Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM.

As discussed elsewhere, I'd eventually like to collect all the integration tests under ./tests, but that can happen in the future.

@tnull tnull merged commit fbe6f29 into lightningdevkit:main Dec 12, 2023
11 of 13 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