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

swap: improve balance tests #1873

Merged
merged 119 commits into from
Nov 5, 2019
Merged

swap: improve balance tests #1873

merged 119 commits into from
Nov 5, 2019

Conversation

mortelli
Copy link
Contributor

@mortelli mortelli commented Oct 10, 2019

The purpose of this PR is to improve the test functions for the peer balances RPCs, in terms of code clarity, tidiness and test cases coverage.

  • The tests are renamed so that they follow the Test + name of the function they are testing heuristic.
  • helper functions are added.
  • some comments added.

@mortelli mortelli added in progress and removed builds on open PR Builds on a PR that is not yet merged. It is blocked until then and the diff won't make sense yet. do-not-merge on hold labels Oct 30, 2019
…e-tests

# Conflicts:
#	swap/protocol.go
#	swap/swap.go
#	swap/swap_test.go
// create a test swap account
swap, clean := newTestSwap(t, ownerKey, nil)
defer clean()
// test previous results are still correct
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the added value of these extra tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the value isn't huge (in fact, it's an arguably somewhat paranoic practice) but since we progressively deal with different peers, it's conceivable that after working with a new one, results from the previous peer could be affected (and therefore, be erroneous).

one could apply this logic for many more cases, though

Copy link
Contributor

@Eknir Eknir left a comment

Choose a reason for hiding this comment

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

Functionally not much changed, but a good cleanup. Thanks for doing this!
In both TestBalance and TestBalances, you add extra test cases in the end, seemingly testing something which you already tested before. I am curious why you would do this.
Looks good to me!

Copy link
Member

@ralph-pichler ralph-pichler left a comment

Choose a reason for hiding this comment

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

LGTM

@mortelli mortelli merged commit 52b83a4 into master Nov 5, 2019
@mortelli mortelli deleted the swap-improve-balance-tests branch November 5, 2019 17:57
@acud acud added this to the 0.5.3 milestone Nov 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants