Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[CO-436] Change integration tests way of confirming transactions #3791

Conversation

paweljakubas
Copy link
Contributor

Description

Here we are switching in a enterprise of confirming the transaction into looking at the status of the transaction. The current tests rely on the confirmation number, which is not correct. The only reason those tests don't fail entirely at this moment is because there is an implicit ordering at the creation_time of the tx. So when the tests take the first tx of the list and check on it, this happens to be the correct tx. But this is very unreliable.

In a nutshell we demand that the transaction's status is either InNewestBlocks or Persisted

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CO-436

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

nix-build -A walletIntegrationTests -o launch_integration_tests --arg useStackBinaries true
./launch_integration_tests

Screenshots (if available)

confirmTransaction transactions txn = do
let txnEntry: _ = filter ( \x -> (txId x) == (txId txn)) transactions
log $ "Resp : " <> ppShowT txnEntry
liftIO $ [txStatus txnEntry] `shouldContain` [InNewestBlocks, Persisted]
Copy link
Contributor

Choose a reason for hiding this comment

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

I read it the other way around. Isn't [InNewestBlocks, Persisted] which should contain [txStatus txnEntry] ?

log $ "Resp : " <> ppShowT txnEntry
liftIO $ txConfirmations txnEntry `shouldNotBe` 0

confirmTransaction resp txn
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather have a naming that matches HSpec's style here. Something like:

txn `shouldBeConfirmed` resp

log = putStrLn . mappend "[TEST-LOG] "

ppShowT :: Show a => a -> Text
ppShowT = fromString . ppShow
Copy link
Contributor

Choose a reason for hiding this comment

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

If those are only used in the above function, let's make them part of a where clause. There's no reason why they should be available for the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made changes as suggested, commits squashed. log, ppShowT are also used in wallet-new/integration/RandomStateWalk.hs

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, we gotta remove this file at some point :| ... But not part of this PR.

Thanks @paweljakubas waiting for the CI and merging if everything's okay.

@paweljakubas paweljakubas force-pushed the paweljakubas/CO-436/rely-on-confirmation-status-in-integration-tests branch 2 times, most recently from de4058e to e003b10 Compare October 26, 2018 14:50
[CO-436] Comply with review suggestions

[CO-436] Applying shouldNotBeConfirmed when sending from locked addresses
@KtorZ
Copy link
Contributor

KtorZ commented Oct 26, 2018

Integration tests now fixed. Good job 👍

@KtorZ KtorZ merged commit 601ee68 into develop Oct 26, 2018
@KtorZ KtorZ deleted the paweljakubas/CO-436/rely-on-confirmation-status-in-integration-tests branch October 26, 2018 15:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants