Skip to content

Add unit test to avoid new string fields #3101

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
Merged
jannotti merged 6 commits into from
Oct 30, 2021
Merged

Add unit test to avoid new string fields #3101

jannotti merged 6 commits into from
Oct 30, 2021

Conversation

@jasonpaulos
Copy link
Contributor

@jasonpaulos jasonpaulos commented Oct 19, 2021 •
edited
Loading

Summary

Add a unit test which fails if any new string types are referenced from bookkeeping.Block.

Closes #3093. See that issue for more context.

Test Plan

This is a test.

@jasonpaulos jasonpaulos marked this pull request as ready for review October 19, 2021 19:43
@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2021 •
edited
Loading

Codecov Report

Merging #3101 (78ec836) into master (c2054e5) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3101      +/-   ##
==========================================
- Coverage   43.68%   43.66%   -0.03%     
==========================================
  Files         390      390              
  Lines       86681    86681              
==========================================
- Hits        37868    37845      -23     
- Misses      42796    42817      +21     
- Partials     6017     6019       +2
Impacted Files Coverage Δ
ledger/roundlru.go 90.56% <0.00%> (-5.67%) ⬇️
network/wsPeer.go 69.86% <0.00%> (-2.94%) ⬇️
catchup/service.go 69.59% <0.00%> (-1.51%) ⬇️
ledger/internal/eval.go 69.94% <0.00%> (-0.27%) ⬇️
ledger/acctupdates.go 64.75% <0.00%> (-0.10%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2054e5...78ec836. Read the comment docs.

@tsachiherman tsachiherman removed their assignment Oct 20, 2021
@tsachiherman tsachiherman self-requested a review October 20, 2021 02:15
tsachiherman
tsachiherman reviewed Oct 20, 2021
View reviewed changes
data/bookkeeping/encoding_test.go Outdated
Copy link
Contributor

@tsachiherman tsachiherman Oct 20, 2021

Choose a reason for hiding this comment

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

would be nicer to have this as Payset\[]SignedTxnWithAD\SignedTxn\Txn\AssetConfigTxnFields, but given that this is a deterministic list, it's quite de-motivating to make the extra effort.

tsachiherman
tsachiherman previously approved these changes Oct 20, 2021
View reviewed changes
Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

looks great.

@jasonpaulos
Copy link
Contributor Author

jasonpaulos commented Oct 20, 2021

I just realized this is missing EvalDelta.GlobalDelta, which is map[string]ValueDelta. Converting this to a draft while I debug.

@jasonpaulos jasonpaulos marked this pull request as draft October 20, 2021 16:35
@jasonpaulos jasonpaulos marked this pull request as ready for review October 20, 2021 16:58
@jasonpaulos
Copy link
Contributor Author

jasonpaulos commented Oct 20, 2021

Found the issue, the seen set was being too aggressive, meaning some paths weren't properly checked. Fixed now.

@tolikzinovyev
Copy link
Contributor

tolikzinovyev commented Oct 20, 2021

Would be nice to do the same for AccountData!

jannotti
jannotti reviewed Oct 26, 2021
View reviewed changes
@jannotti jannotti merged commit 385afd5 into algorand:master Oct 30, 2021
@jasonpaulos jasonpaulos deleted the no-string-fields branch November 1, 2021 15:30
cce pushed a commit to cce/go-algorand that referenced this pull request Nov 3, 2021
@jasonpaulos @cce
Add unit test to avoid new string fields (algorand#3101)
2a255c2 
* Add unit test to check for string fields

* Fix seen set and add exceptions for state deltas

* Also check AccountData

* Minor rename

* Add license

* Accidentally pushed to origin, need new hash
@egieseke egieseke mentioned this pull request Nov 23, 2021
Merged
@algobarb algobarb mentioned this pull request Dec 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jannotti jannotti jannotti approved these changes

+1 more reviewer

@tsachiherman tsachiherman tsachiherman left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jasonpaulos jasonpaulos

Labels

Team Scytale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Avoid using string fields for binary data

5 participants

@jasonpaulos @codecov-commenter @tolikzinovyev @jannotti @tsachiherman