Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
chore(core): remove legacy "02" tests #3677
chore(core): remove legacy "02" tests #3677
Changes from all commits
b3aaeee
037e41b
607f91c
ecd2b90
6321874
56749c5
788da2b
e71f398
fc98bad
a8cb9f0
0b3769f
ef1e61b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this is not a test
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.
How do you mean? There's a code branch in
checkAndDeletePaymentForHash
that persists an lnPayment if it didn't previously exist in LnPaymentsRepository that I explicitly wanted to testThere 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.
I review bottom -> up, there is no need to have 2 tests for this, is just add
in second test but it is up to you
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.
I'd prefer to have them separate because the 2nd test needs to have an explicit step that marks
isCompleteRecord
as true and I bring that in manually, and if I tried to not do that then the test becomes more complicated because later tests would rely on prior state in the testThere 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.
this is what needs to be tested... deleteLnPaymentsBefore should persist it in lnpayment if it does not exists
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.
Ok I don't understand 😅 ... isn't that what this tests
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.
is this not done by
await Lightning.deleteLnPaymentsBefore(timestamp2Months)
line 89?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.
Ah nope, it persists it but then it won't get
isCompleteRecord
property marked astrue
unless we runupdateLnPayments
which is a lot more than we need just to test thatdeletePaymentByHash
gets hit under the right conditionsSo I just manually setup the lnPayment in the repository so that the use-case can continue on to what we want to test here