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 integration tests for error messages from balance endpoint #2987

Merged
merged 9 commits into from
Nov 8, 2021

Conversation

piotr-iohk
Copy link
Contributor

@piotr-iohk piotr-iohk commented Oct 21, 2021

Issue Number

ADP-1241
ADP-1225

Comments

This PR adds:

  • integration test coverage for the numeric/variable parts of a collateral selection error.
  • integration test coverage for error messages from the balance point. (These are currently marked as pending, as we expect to improve these errors.)

@piotr-iohk piotr-iohk force-pushed the piotr/balance-ep/error-message branch from e53e75c to 5f778f0 Compare October 22, 2021 13:03
, expectErrorMessage errMsg403Fee
]

it "TRANS_NEW_BALANCE_02c - Cannot balance on when I cannot afford collateral" $ \ctx -> runResourceT $ do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly ADP-1241 case @jonathanknowles.

Copy link
Contributor

Choose a reason for hiding this comment

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

@piotr-iohk I've prepared some enhancements to this test in the following branch:
https://github.com/input-output-hk/cardano-wallet/compare/jonathanknowles/adp-1225/test-enhancements

Will make this into a PR tomorrow morning!

Copy link
Contributor

Choose a reason for hiding this comment

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

Update, I've added these commits to this PR.

@jonathanknowles jonathanknowles force-pushed the piotr/balance-ep/error-message branch from 5f778f0 to 82e025c Compare November 2, 2021 05:05
Piotr Stachyra and others added 8 commits November 8, 2021 07:48
We:

- sort the largest available combination of pure ada entries into
  ascending order, which eases testing.
- use a more natural list format, which adds a space after the comma (to
  make the output more readable).
This will allow us to test the output of a failed collateral selection
where the largest available combination of pure ada UTxOs has more than
one entry.
It's important to test the variable parts of this error so that we we
can verify that the user is being presented with sensible advice.

It's also important to verify the numerical values as a sanity check
that they are sensible.

Note that our property tests all use simulated cost models, so it's
doubly important to have /something/ in the integration test suite to
act as a check for sensible values.
This is to account for recent fixes to the fee calculation logic within
the `balanceTransaction` function, in particular:

#2989
@jonathanknowles jonathanknowles self-assigned this Nov 8, 2021
@jonathanknowles jonathanknowles changed the title FIXME ADP-1225: Error messages from balance ep Add integration tests for error messages from balance endpoint Nov 8, 2021
We expect to improve these errors in a future PR.
@jonathanknowles jonathanknowles force-pushed the piotr/balance-ep/error-message branch from 82e025c to 83fcca6 Compare November 8, 2021 08:24
@jonathanknowles jonathanknowles self-requested a review November 8, 2021 08:25
@jonathanknowles
Copy link
Contributor

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 8, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 72e7e4f into master Nov 8, 2021
@iohk-bors iohk-bors bot deleted the piotr/balance-ep/error-message branch November 8, 2021 10:56
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.

2 participants