cleverca22 commented 4 years ago

Description

Linked issue

Type of change

[ ] 🐞 Bug fix (non-breaking change which fixes an issue)
[ ] 🛠 New feature (non-breaking change which adds functionality)
[ ] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
[ ] 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
[ ] 🔨 New or improved tests for existing code
[ ] ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

[ ] I have read the style guide document, and my code follows the code style of this project.
[ ] If my code deals with exceptions, it follows the guidelines.
[ ] I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
[ ] CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

[ ] I have added tests to cover my changes.
[ ] All new and existing tests passed.

QA Steps

Screenshots (if available)

How to merge

Send the message bors r+ to merge this PR. For more information, see docs/how-to/bors.md.

tatyanavych commented 4 years ago

@cleverca22 Please, add CHANGELOG.md entry under Unreleased in Features section (the section does not exist - needs to be created). Could you also add Fixes section below for the existed entry? So to have:

Unreleased

Features

A new endpoint... (CBR-533 #<PR#>)

Fixes

Correct change category...

cleverca22 commented 4 years ago

@KtorZ without some chain state to fire up the wallet, i don't know how to test this endpoint fully in an automated manner

all other review comments should be good

KtorZ commented 4 years ago

@cleverca22 For the testing, have a look at some integration test scenarios (e.g. https://github.com/input-output-hk/cardano-sl/blob/develop/wallet/test/integration/Test/Integration/Scenario/Wallets.hs#L579-L601)

You should be able to do something like:

    scenario "Can query wallet id and balance using only its mnemonic" $
            fixture <- setup $ defaultSetup
                & initialCoins .~ [14]

            successfulRequest $ Client.deleteWallet
                $- (fixture ^. wallet . walletId)

            response <- request $ Client.calculateMnemonic 
                $- (fixture ^. backupPhrase)
                $- (Just True) 

            verify response
                [ expectFieldEqual walletId (fixture ^. wallet . walletId)
                , expectFieldEqual maybeBalance (Just 14)
                ]

+/- the compilation errors and testing with various scenarios (with the query parameter, without, testing on a wallet with no funds etc ..)

disassembler commented 4 years ago

bors r+

iohk-bors[bot] commented 4 years ago

:-1: Rejected by too few approved reviews