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.

Sign up for GitHub

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

Jump to bottom

FastCatchup - add unit testing for ledgerFetcher #1147

Merged
merged 202 commits into from
Jun 9, 2020
Merged

FastCatchup - add unit testing for ledgerFetcher #1147

merged 202 commits into from
Jun 9, 2020

Conversation

tsachiherman
Copy link
Contributor

@tsachiherman tsachiherman commented Jun 9, 2020 •
edited
Loading

Summary

Add testing for the ledgerFetcher

Notes

  1. Refactor ledger.CatchpointCatchupAccessor into an Interface, so it could be mocked out and abstracted.
  2. Create a mockCatchpointCatchupAccessor.go
  3. Create a unit tests for ledgerFetcher which covers all the error execution paths.

@tsachizehub
Create catchpoint writer.
88b8f1d
@tsachizehub
Merge branch 'master' into tsachi/fastcatchup
ffa6dc8
@tsachizehub
add config file.
c2036f4
@tsachizehub
update to go 1.13
c2af19b
@tsachizehub
Fix few bugs.
276b7c9
@tsachizehub
Few config changes.
340a516
@tsachizehub
better error reporting.
68ad835
@tsachizehub
test
e51d0f2
@tsachizehub
Few more changes.
91fa20d
@tsachizehub
Merge branch 'master' into tsachi/fastcatchup
4f50c12
@tsachizehub
step further.
39f919d
@tsachizehub
Merge branch 'master' into tsachi/fastcatchup
15f1ea0
@tsachizehub
Few bug fixes.
9fdc1ba
@tsachizehub
few aligments.
75995cb
@tsachizehub
update.
bf82d31
@tsachizehub
some more.
8b98645
@tsachizehub
Merge branch 'master' into tsachi/fastcatchup
5acc8fa
@tsachizehub
Small step.
e756dcb
@tsachizehub
fix compilation.
1c2b961
@tsachizehub
fix compilation.
bdc531d
@tsachizehub
start/abort func.
100e89d
@tsachizehub
Merge branch 'master' into tsachi/fastcatchup
01184ca
@tsachizehub
Few more changes.
98b6ccb
@tsachizehub
few more changes.
65ca248
@tsachizehub
add missing file.
07ccaa3
@tsachizehub
few more changes.
78a6fd1
@tsachizehub
missing file.
bab9dc8
@tsachizehub
rollback change.
c7b3952
@tsachizehub
Fix failed unit test.
f3ffda8
@tsachizehub
step
102d492
tsachiherman added 17 commits May 31, 2020 15:49
@tsachiherman
Set CatchpointInterval to 10000
fbc8c27 
Fix typos
@tsachiherman
Add watchdogStreamReader documentation
7093f39
@tsachiherman
Merge branch 'master' into tsachi/fastcatchup
d588919
@tsachiherman
Fix datarace when writing a catchpoint label.
609a79d
@tsachiherman
Add a unit test to verify the datarace fix.
1da53a5
@tsachiherman
Fix close sync bug in TestAcctUpdatesFastUpdates, and add safeguards …
8aca602 
…against out-of-order committedUpTo calls. ( based on unit test only. no actual required by production code ).
@tsachiherman
Fix two more dataraces during catchup mode switch.
b7ce9cb
@tsachiherman
Merge branch 'master' into tsachi/fastcatchup
fd80deb
@tsachiherman
Make sure node.Status() doesn't access the ledger in catchpoint catch…
b5989c2 
…up mode.
@tsachiherman
Merge branch 'master' into tsachi/fastcatchup
d7fc92e
@tsachiherman
Add unit test TestLargeAccountCountCatchpointGeneration
124b661
@tsachiherman
Add telemetry & logging on catchpoint file creation.
101bc4a
@tsachiherman
Remove temporary catchpoints directroy.
2a80508
@tsachiherman
Add TestBasicCatchpointWriter unit test
da257a1
@tsachiherman
Add ledgerFetcher error case testing ( phase 1 )
3e1f179
@tsachiherman
Add missing components/mocks/mockCatchpointCatchupAccessor.go
a84ee55
@tsachiherman
Merge branch 'master' into tsachi/fastcatchup3
0c94863
@tsachiherman tsachiherman marked this pull request as ready for review June 9, 2020 17:43
@tsachiherman tsachiherman self-assigned this Jun 9, 2020
algonautshant
algonautshant approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@algonautshant algonautshant left a comment

Choose a reason for hiding this comment

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

Looks good.

algorandskiy
algorandskiy approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Would be nice to have a happy case test as well

wjurayj-algo
wjurayj-algo reviewed Jun 9, 2020
View reviewed changes
Copy link
Contributor

@wjurayj-algo wjurayj-algo left a comment

Choose a reason for hiding this comment

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

Mostly looks good to my eyes, my only real concern is the pointer return discrepancy I commented.

catchup/ledgerFetcher_test.go Show resolved Hide resolved
ledger/catchupaccessor.go Show resolved Hide resolved
wjurayj-algo
wjurayj-algo approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@wjurayj-algo wjurayj-algo left a comment

Choose a reason for hiding this comment

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

All looks good!

@tsachiherman
Copy link
Contributor Author

Would be nice to have a happy case test as well

definitely. I'm planning to get there. however, I'm trying to prioritize the use cases which aren't covered at all by other tests first. The success case is covered by the e2e test, so it "defaults" to have a lower priority.

@tsachiherman tsachiherman merged commit 13b7864 into algorand:master Jun 9, 2020
@tsachiherman tsachiherman deleted the tsachi/fastcatchup3 branch June 9, 2020 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjurayj-algo wjurayj-algo wjurayj-algo approved these changes

@algorandskiy algorandskiy algorandskiy approved these changes

@algonautshant algonautshant algonautshant approved these changes

Assignees

@tsachiherman tsachiherman

Labels
Infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tsachiherman @algonautshant @algorandskiy @wjurayj-algo @tsachizehub