-
Notifications
You must be signed in to change notification settings - Fork 14
Create new v2_to_v2 integration test #85
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
Conversation
8514ad5
to
af5c1ff
Compare
9bafb19
to
ab25849
Compare
549c956
to
0262683
Compare
85559df
to
8c745bc
Compare
5b754af
to
aa6c733
Compare
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.
bda8b68
to
4ef75b6
Compare
This comment was marked as resolved.
This comment was marked as resolved.
82d4418
to
4c6fd51
Compare
In case this build fails (Success pls???) Is there a good reason not to use |
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 |
a626dd8
to
1364c24
Compare
There was a problem hiding this 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.
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) | ||
}) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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.
1364c24
to
83359b5
Compare
How come the mac version is unable to use redis provided by payjoin-test-utils? |
There was a problem hiding this 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.
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. |
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.
I agree though that it would be nice to not get these errors though |
Creates new v2_to_v2 integration test