Skip to content

Conversation

benalleng
Copy link
Contributor

@benalleng benalleng commented Apr 29, 2025

Creates new v2_to_v2 integration test

@benalleng benalleng force-pushed the create-new-v2-tests branch from 8514ad5 to af5c1ff Compare April 29, 2025 13:48
@benalleng benalleng changed the title Create new v2 tests Create new v2_to_v2 integration test Apr 29, 2025
@benalleng benalleng force-pushed the create-new-v2-tests branch 6 times, most recently from 9bafb19 to ab25849 Compare April 30, 2025 18:00
@DanGould DanGould mentioned this pull request Apr 30, 2025
@benalleng benalleng force-pushed the create-new-v2-tests branch 5 times, most recently from 549c956 to 0262683 Compare May 1, 2025 12:51
@benalleng benalleng force-pushed the create-new-v2-tests branch 13 times, most recently from 85559df to 8c745bc Compare May 7, 2025 15:34
@benalleng benalleng force-pushed the create-new-v2-tests branch 2 times, most recently from 5b754af to aa6c733 Compare May 8, 2025 16:31
benalleng and others added 4 commits May 9, 2025 12:51
We need more control over psbts in the integration tests so
I am using my branch of bitcoin_ffi until we have that PR merged
into main.
Callback methods meant to be overridden by the foreign languange should
be exposed with
[`with_foreign`](https://mozilla.github.io/uniffi-rs/0.27/foreign_traits.html#1-define-a-rust-trait)
This adds updated integration tests to work with 0.23 and beyond as it
now utilizes the updated payjoin_test_utils.
Using filenames that start with test will allow us to run a more
seamless workflow.
@benalleng benalleng force-pushed the create-new-v2-tests branch from bda8b68 to 4ef75b6 Compare May 9, 2025 16:51
@benalleng

This comment was marked as resolved.

@benalleng benalleng force-pushed the create-new-v2-tests branch 3 times, most recently from 82d4418 to 4c6fd51 Compare May 12, 2025 15:31
@DanGould
Copy link
Contributor

it does not have access to the same environments by default as ubunut-latest

In case this build fails (Success pls???)

Is there a good reason not to use ubuntu-latest?

@benalleng
Copy link
Contributor Author

it does not have access to the same environments by default as ubunut-latest

In case this build fails (Success pls???)

Is there a good reason not to use ubuntu-latest?

Not really, I was going to try that soon if this current attempt doesn't work, I was just hoping to keep the minimal manylinux image if possible, it might be but perhaps its not worth it

@benalleng benalleng force-pushed the create-new-v2-tests branch 6 times, most recently from a626dd8 to 1364c24 Compare May 12, 2025 18:10
@DanGould DanGould self-requested a review May 13, 2025 01:06
Copy link
Contributor

@DanGould DanGould left a comment

Choose a reason for hiding this comment

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

cACK 1364c24

Does CI work on commits before the one I cACKed? Or would there be errors if not for the CI changes in the final commit? That's my only real concern assuming integration testing is operating as I imagine it is. The other comments are just notes for the future.

Comment on lines +192 to +196
pub fn fetch_ohttp_keys(&self) -> Result<crate::OhttpKeys, crate::io::IoError> {
let runtime = RUNTIME.lock().expect("Lock should not be poisoned");
runtime.block_on(async {
self.0.lock().await.fetch_ohttp_keys().await.map_err(Into::into).map(Into::into)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

The repeated let runtime =, runtime.block_on makes me think this could be dried up by a delarative macro. But there's no rush on this, it can get in after this is merged, just a prettier way to write.

toml==0.10.2
yapf==0.43.0

httpx==0.28.1
Copy link
Contributor

Choose a reason for hiding this comment

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

very pleased we're able to use httpx here instead of exposing rust's reqwest. I'm curious, though not particularly eager, to see if the native http clients for some foreign language can replace fetch_ohttp_keys so we don't have to ship a whole rust TLS stack with our binaries, too.

@benalleng
Copy link
Contributor Author

021dab3 causes a CI falure as the integration test has been added to the workflow there but the followup changes to add support redis are not in until 1364c24

@DanGould
Copy link
Contributor

021dab3 causes a CI falure as the integration test has been added to the workflow there but the followup changes to add support redis are not in until 1364c24

shall those commits then be squashed or tests ignored so that each commit in turn passes CI and git bisect works?

@benalleng
Copy link
Contributor Author

benalleng commented May 13, 2025

I think we should just squash them such that the integration test doesn't get added to the Ci until the redis compatibility is also included as 021dab3 really doesn't do anything other than cause a CI failure with the new tests added

Now that the test is working we should add it to the workflow. I have
left this as a seperate commit in case it adds too much time to the
workflows.

By adding ubuntu latest for linux and a special workflow command for
macos we are able to enable redis to create a compatible workflow for
running the new integration tests.
@benalleng benalleng force-pushed the create-new-v2-tests branch from 1364c24 to 83359b5 Compare May 13, 2025 17:00
@DanGould
Copy link
Contributor

How come the mac version is unable to use redis provided by payjoin-test-utils?

Copy link
Contributor

@DanGould DanGould left a comment

Choose a reason for hiding this comment

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

tACK 83359b5

I ran the python test on my local machine.

I'm still confused why redis setup is re-scripted in the CI yml though. Why not use the payjoin-test-utils docker version so that it's the same as rust? Perhaps this needs to be done in a follow up.

@DanGould
Copy link
Contributor

DanGould commented May 13, 2025

Hmmm but it also seems like there might be a silent test failure in the python tests. Look at the 'Run tests' section

 OHTTP relay binding to port [::]:33681
2025-05-13T17:02:36.074649Z ERROR ohttp_relay::bootstrap::connect: server io error: Transport endpoint is not connected (os error 107)
test_integration_v2_to_v2 (test.test_payjoin_integration_test.TestPayjoin.test_integration_v2_to_v2) ... Executing <Task pending name='Task-2' coro=<TestPayjoin.test_integration_v2_to_v2() running at /home/runner/work/payjoin-ffi/payjoin-ffi/python/test/test_payjoin_integration_test.py:92> cb=[_run_until_complete_cb() at /opt/hostedtoolcache/Python/3.13.3/x64/lib/python3.13/asyncio/base_events.py:181] created at /opt/hostedtoolcache/Python/3.13.3/x64/lib/python3.13/asyncio/runners.py:100> took 3.964 seconds
2025-05-13T17:02:38.449256Z ERROR payjoin_directory: Error serving connection: hyper::Error(Shutdown, Os { code: 107, kind: NotConnected, message: "Transport endpoint is not connected" })
Executing <Task finished name='Task-2' coro=<TestPayjoin.test_integration_v2_to_v2() done, defined at /home/runner/work/payjoin-ffi/payjoin-ffi/python/test/test_payjoin_integration_test.py:71> result=None created at /opt/hostedtoolcache/Python/3.13.3/x64/lib/python3.13/asyncio/runners.py:100> took 0.127 seconds
ok

is this a silent failure or some loop that's waiting and errors the first go around? I think it might just be waiting but ERROR prints are frightening.

@benalleng
Copy link
Contributor Author

benalleng commented May 13, 2025

some loop that's waiting and errors the first go around?

I'm pretty sure its the latter as the test still resolves to the end to an ok response with all the asserts we have in place, and at least one of these are the same errors on the rust-payjoin.

Database running on 127.0.0.1:32865
Directory server binding to port [::]:45329
OHTTP relay binding to port [::]:37679
session: Receiver {
    context: SessionContext {
        ...
    },
}
2025-05-13T17:49:46.254400Z ERROR ohttp_relay::bootstrap::connect: server io error: Transport endpoint is not
 connected (os error 107)
test integration::v2::v2_to_v2 ... ok

I agree though that it would be nice to not get these errors though

@DanGould DanGould merged commit d3a2b6f into LtbLightning:main May 13, 2025
10 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.

4 participants