#517 AccountDeleteIT

[agutierrez0]

Added integration tests for AccountDelete transaction. Closes #517

• Covered all error cases except tefTOO_BIG, object submitted to the ledger would have been the same as other tests so wanted to avoid "testing the ledger" vs "testing AccountDelete".
• Moved getMinExpirationTime function into AbstractIT as I used it when creating an EscrowCreate transaction for the finishAfter parameter.
• Used DisabledIf annotation to only run successful test on local rippled nodes, this was because AccountDelete requires 256 ledgers to pass before being working successfully and I was using an XrplAdminClient instance to accept the ledgers.
• Exposed new method acceptLedger in RippleDContainer/LocalRippledEnvironment in order to accept ledger at the speed of code compared to the interval speed we set for LEDGER_ACCEPTOR.

First time contributing to open source code, open to any feedback 👍🏽

[sappenin]

LGTM, thanks for the submission, and happy to have you contributing @agutierrez0 !!

[sappenin]

Oops, I spoke too soon -- looks like there's a checkstyle issue in the build. Here's the error:

[WARN] /home/runner/work/xrpl4j/xrpl4j/xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/AccountDeleteIT.java:278:5: Distance between
 variable 'receiverAccount' declaration and its first usage is 4, but allowed 3.  Consider
 making that variable final if you still need to store its value in advance (before method 
calls that might have side effects on the original value).
[VariableDeclarationUsageDistance]

[agutierrez0]

Oops, I spoke too soon -- looks like there's a checkstyle issue in the build. Here's the error:

[WARN] /home/runner/work/xrpl4j/xrpl4j/xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/AccountDeleteIT.java:278:5: Distance between
 variable 'receiverAccount' declaration and its first usage is 4, but allowed 3.  Consider
 making that variable final if you still need to store its value in advance (before method 
calls that might have side effects on the original value).
[VariableDeclarationUsageDistance]

@sappenin Hmm yeah I saw this error too earlier and pushed this commit to address it: ee7029d

I'm not seeing any of the jobs fail and I am not able to reproduce the error when building locally, is there any way I could see which job threw the error? Not sure how to "re-run" the pipeline like with GitLab 😅

Thanks for taking the time to review

[sappenin]

Sure thing - you can see the jobs that get run here (see Actions tab above). I've kicked off another build to see what happens.

Actually, I think it doesn't like declaring the receiverAccount variable so far away from its first usage.

[agutierrez0]

Thanks for rerunning @sappenin, just made one more change 1b17c55 moving that receiverAccount variable down. i saw the error message suggest making it final and gave that a shot, but went with moving it down instead. updated comments as well.

will my pushes automatically trigger another build?

[sappenin]

Sorry @agutierrez0, I missed this one -- just triggered a new build (see here).

[agutierrez0]

No problem & thanks @sappenin 👍🏽 , i see my build failed again but it has to do with PaymentChannelIT. Could my changes have somehow impacted those integration tests?

[sappenin]

i see my build failed again but it has to do with PaymentChannelIT. Could my changes have somehow impacted those integration tests?

Unclear, seems unlikely. I've kicked-off another build, so let's see what happens.

I presume all ITs pass locally?

[agutierrez0]

thanks for the approve @nkramer44 👍🏽

i see that the build continues to fail. locally the ITs pass:

is there anything i can do to address the issues in the pipeline build?

[sappenin]

@agutierrez0 It seems like the broken build isn't due to your changes, so I'm going to merge now and let's see what happens to the build on main. For now, main is mostly working (see here), so let's see what happens.

[agutierrez0]

@sappenin , thanks for the help triaging this. @nkramer44 let me know that the @DisabledIf annotation might not be working as expected since it looks like tests are running on testnet and devnet, and since I am doing this manual accepting of the ledger, it might be erroring out there.

I noticed everywhere else @DisabledIf was being used, it was for the entire class, so I moved mine to be for the entire class as well. Would you mind running another build to see if this could address the issue?

[sappenin]

Would you mind running another build to see if this could address the issue?

Sure thing - just approved it.