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 tests for receiving and paying invoices when position is open #880

Merged
merged 8 commits into from
Jul 5, 2023

Conversation

klochowicz
Copy link
Contributor

@klochowicz klochowicz commented Jul 3, 2023

Add test coverage to a ticket that got recently closed - we should test that we can receive and send payments

Also, return error in the API if a payment failed

@klochowicz klochowicz marked this pull request as draft July 3, 2023 03:32
@klochowicz klochowicz self-assigned this Jul 3, 2023
@klochowicz klochowicz force-pushed the chore/add-tests-for-payments-when-position-open branch 2 times, most recently from 4f6a8aa to 5cd8caa Compare July 3, 2023 06:07
@klochowicz klochowicz marked this pull request as ready for review July 3, 2023 06:11
@klochowicz klochowicz requested a review from da-kami July 3, 2023 07:30
@klochowicz klochowicz requested a review from holzeis July 4, 2023 00:32
@klochowicz klochowicz force-pushed the chore/add-tests-for-payments-when-position-open branch from 5cd8caa to 4c86842 Compare July 4, 2023 01:01
crates/tests-e2e/tests/payments_whilst_open_position.rs Outdated Show resolved Hide resolved
crates/tests-e2e/src/coordinator.rs Outdated Show resolved Hide resolved
crates/ln-dlc-node/src/node/invoice.rs Outdated Show resolved Hide resolved
crates/tests-e2e/src/logger.rs Outdated Show resolved Hide resolved
crates/tests-e2e/src/app.rs Outdated Show resolved Hide resolved
crates/tests-e2e/tests/send_payment_when_open_position.rs Outdated Show resolved Hide resolved
mobile/native/src/api.rs Outdated Show resolved Hide resolved
@klochowicz klochowicz force-pushed the chore/add-tests-for-payments-when-position-open branch 6 times, most recently from d9ef756 to ab9fdbf Compare July 4, 2023 06:49
@klochowicz
Copy link
Contributor Author

@holzeis it failed again on CI - I think I will add a getter to see open positions and wait_until! there is one on the coordinator.

@holzeis
Copy link
Contributor

holzeis commented Jul 4, 2023

@holzeis it failed again on CI - I think I will add a getter to see open positions and wait_until! there is one on the coordinator.

I think that would be useful for many reasons 👍

Starting from a funded state or an open position state is common.
@klochowicz klochowicz force-pushed the chore/add-tests-for-payments-when-position-open branch from 4d1ccd5 to ac90c99 Compare July 5, 2023 04:34
We used to return success on the API when we knew the payment failed due to
any of the RetryableSendFailure reasons.

Since we know the payment failed, it's best to just return the error, so the
caller can do something about it.
Keep capturing in the app, don't capture backtrace in the tests.
@klochowicz
Copy link
Contributor Author

@holzeis it failed again on CI - I think I will add a getter to see open positions and wait_until! there is one on the coordinator.

I think that would be useful for many reasons 👍

I will move that into a follow-up, as I don't want to block anyone else writing e2e tests (I know @da-kami is about to do some)

@klochowicz
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 5, 2023

Canceled.

Contrary to a local run the CI is skipping the direct route to payee for unknown reasons.
That's why we are relaxing the balance assertion to cater for the fees being substracted or not.

This should be changed once #883 is fixed.
Receiving a payment can fail if we get it before a position got open.

Ideally, we should move towards checking whether coordinator opened a position
instead of doing a sleep.
@klochowicz
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Jul 5, 2023
880: Add tests for receiving and paying invoices when position is open r=klochowicz a=klochowicz

Add test coverage to a ticket that got recently closed - we should test that we can receive and send payments

Also, return error in the API if a payment failed


Co-authored-by: Mariusz Klochowicz <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jul 5, 2023

Build failed:

@klochowicz
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Jul 5, 2023
880: Add tests for receiving and paying invoices when position is open r=klochowicz a=klochowicz

Add test coverage to a ticket that got recently closed - we should test that we can receive and send payments

Also, return error in the API if a payment failed


Co-authored-by: Mariusz Klochowicz <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jul 5, 2023

Build failed:

It was just an early showcase of the framework.
@klochowicz klochowicz force-pushed the chore/add-tests-for-payments-when-position-open branch from 20adf87 to 8fa4c86 Compare July 5, 2023 06:51
@klochowicz
Copy link
Contributor Author

bors r+

@da-kami
Copy link
Contributor

da-kami commented Jul 5, 2023

Build failed:

* [e2e-tests](https://github.com/get10101/10101/actions/runs/5461131629/jobs/9938791983)

Was this test flaky - did you change something?

@bors
Copy link
Contributor

bors bot commented Jul 5, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 08bd9b9 into main Jul 5, 2023
7 checks passed
@bors bors bot deleted the chore/add-tests-for-payments-when-position-open branch July 5, 2023 07:18
@klochowicz
Copy link
Contributor Author

Build failed:

* [e2e-tests](https://github.com/get10101/10101/actions/runs/5461131629/jobs/9938791983)

Was this test flaky - did you change something?

see your github notifications. it's the JIT payment feature being flaky - there's an open ticket to address it #883

@holzeis
Copy link
Contributor

holzeis commented Jul 5, 2023

Build failed:

* [e2e-tests](https://github.com/get10101/10101/actions/runs/5461131629/jobs/9938791983)

Was this test flaky - did you change something?

see your github notifications. it's the JIT payment feature being flaky - there's an open ticket to address it #883

The flaky test has nothing to do with #883 - It's the new test that has been added with that change that failed.

@klochowicz given that the e2e tests are flaky like the "slow running" tests. Should we revert #888 ?

@da-kami
Copy link
Contributor

da-kami commented Jul 7, 2023

Build failed:

* [e2e-tests](https://github.com/get10101/10101/actions/runs/5461131629/jobs/9938791983)

Was this test flaky - did you change something?

see your github notifications. it's the JIT payment feature being flaky - there's an open ticket to address it #883

The flaky test has nothing to do with #883 - It's the new test that has been added with that change that failed.

@klochowicz given that the e2e tests are flaky like the "slow running" tests. Should we revert #888 ?

Thanks for pointing this out :)
I would monitor it for a bit and then decide - if they are flaky we can consider reverting, or just improve them.

@klochowicz
Copy link
Contributor Author

Build failed:

* [e2e-tests](https://github.com/get10101/10101/actions/runs/5461131629/jobs/9938791983)

Was this test flaky - did you change something?

see your github notifications. it's the JIT payment feature being flaky - there's an open ticket to address it #883

The flaky test has nothing to do with #883 - It's the new test that has been added with that change that failed.

@klochowicz given that the e2e tests are flaky like the "slow running" tests. Should we revert #888 ?

IMHO ideally we should make them stable again - which means tidying up recent workarounds etc, and ensuring we always get properly funded.

however, if it gets in the way of development, of course we can disable it on bors.

@holzeis
Copy link
Contributor

holzeis commented Jul 10, 2023

The flaky test has nothing to do with #883 - It's the new test that has been added with that change that failed.
@klochowicz given that the e2e tests are flaky like the "slow running" tests. Should we revert #888 ?

IMHO ideally we should make them stable again - which means tidying up recent workarounds etc, and ensuring we always get properly funded.

however, if it gets in the way of development, of course we can disable it on bors.

what workarounds are you referring to? If its the additional mining when funding the app, we might be able to remove that, because of #913. But, that shouldn't have an impact on the flakiness of the tests.. On the contrary, I only added that to remove the flakiness of that test.

@klochowicz
Copy link
Contributor Author

The flaky test has nothing to do with #883 - It's the new test that has been added with that change that failed.
@klochowicz given that the e2e tests are flaky like the "slow running" tests. Should we revert #888 ?

IMHO ideally we should make them stable again - which means tidying up recent workarounds etc, and ensuring we always get properly funded.
however, if it gets in the way of development, of course we can disable it on bors.

what workarounds are you referring to? If its the additional mining when funding the app, we might be able to remove that, because of #913. But, that shouldn't have an impact on the flakiness of the tests.. On the contrary, I only added that to remove the flakiness of that test.

FWIW, I've never seen any of these tests failing before the PRs with JIT channel fees got merged. This might be of course due to the flakiness of the test, but also it might be that we have a problem in our code.

the other one which we should address, and was referring to was #893 - to exclude the possibility that it's the fault of the just fund (which fails often on some machines).

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