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

chore(pkg/scale): return error if Result already has an assigned value #2143

Merged
merged 8 commits into from
Jan 11, 2022
Merged

chore(pkg/scale): return error if Result already has an assigned value #2143

merged 8 commits into from
Jan 11, 2022

Conversation

EclesioMeloJunior
Copy link
Member

@EclesioMeloJunior EclesioMeloJunior commented Jan 5, 2022 •
edited
Loading

Changes

  • Add new error ErrResultAlreadySet
  • Inside Set function check if result.mode is not Unset then return the error

Tests

go test -run ^TestResult_UseSetMoreThanOnce$ ./pkg/scale/...

Issues

Primary Reviewer

qdm12
qdm12 reviewed Jan 6, 2022
View reviewed changes
Copy link
Contributor

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

What's the reason to prevent a subsequent Set() on a result?

pkg/scale/result.go Outdated Show resolved Hide resolved
pkg/scale/result.go Outdated Show resolved Hide resolved
pkg/scale/result_test.go Outdated Show resolved Hide resolved
@EclesioMeloJunior
Copy link
Member Author

@qdm12 there's a slack thread about this issue but basically, if the result.Set is called for the first time with mode Ok and the next time with mode Err the function doesn't replace the previous value. @timwu20 suggested to:

I would probably err on the side of throwing an error for a result that has already been set.

@jimjbrettj
Copy link
Contributor

lgtm!

@codecov
Copy link

codecov bot commented Jan 11, 2022

Codecov Report

Merging #2143 (be6da7b) into development (ccf12de) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #2143      +/-   ##
===============================================
- Coverage        61.56%   61.55%   -0.02%     
===============================================
  Files              213      213              
  Lines            27448    27500      +52     
===============================================
+ Hits             16898    16927      +29     
- Misses            8683     8704      +21     
- Partials          1867     1869       +2
Flag Coverage Δ
unit-tests 61.55% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/scale/result.go 97.14% <100.00%> (+0.12%) ⬆️
dot/network/message_cache.go 58.33% <0.00%> (-8.34%) ⬇️
lib/trie/database.go 47.86% <0.00%> (-4.47%) ⬇️
lib/grandpa/message_tracker.go 66.15% <0.00%> (-4.34%) ⬇️
lib/grandpa/message_handler.go 63.50% <0.00%> (-0.93%) ⬇️
dot/network/light.go 85.25% <0.00%> (-0.80%) ⬇️
lib/trie/child_storage.go 44.26% <0.00%> (ø)
lib/grandpa/vote_message.go 81.96% <0.00%> (ø)
lib/runtime/storage/trie.go 53.89% <0.00%> (ø)
lib/grandpa/grandpa.go 59.05% <0.00%> (+0.21%) ⬆️
... and 5 more

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 e7c964e...be6da7b. Read the comment docs.

@EclesioMeloJunior EclesioMeloJunior merged commit 05e9731 into development Jan 11, 2022
@EclesioMeloJunior EclesioMeloJunior deleted the eclesio/scale-result-set branch January 11, 2022 20:45
timwu20 pushed a commit that referenced this pull request Jan 12, 2022
@EclesioMeloJunior @timwu20
chore(pkg/scale): return error if Result already has an assigned va…
25cd1fc 
…lue (#2143)

* fix: return error if result already has an assigned value

* chore: include unit test

* chore: fix typo `ErrResultAlreadySet`

* chore: remove unneeded `require.Error`

* chore: fix undeclared name

* chore: remove packaged scope var to avoid problems with result type

* chore: fix result.Set error at offchain test
jimjbrettj added a commit that referenced this pull request Jan 14, 2022
@jimjbrettj @EclesioMeloJunior
Jimmy/babe unit tests (#2127)
e1f7f96 
* chore(pkg/scale): return error if `Result` already has an assigned value  (#2143)

* fix: return error if result already has an assigned value

* chore: include unit test

* chore: fix typo `ErrResultAlreadySet`

* chore: remove unneeded `require.Error`

* chore: fix undeclared name

* chore: remove packaged scope var to avoid problems with result type

* chore: fix result.Set error at offchain test

* init

* migrate current tests to integration tests

* fix linting

* test line endings

* remove test comments

* test getSecondarySlotAuthor

* secondary unit tests

* calculate threshold tests

* remove logs

* test checkPrimaryThreshold

* test babe/crypto

* getAuthorityIndex test

* babe mocks with mockgen

* test verifyPrimarySlotWinner

* test verifyPreruntime Digest

* WIP/Test cases for verify authorship right

* finish hitting test cases for equivocation

* clean up naming for verifyBlockAuthor test

* create helper for making verifier

* use helpers to clean up verifyAuthorship test

* clean up mocks

* getConfigData test

* test getVerifierInfo

* remove logs

* test first block cases for block verification

* test VerifyBlock

* finish first half of babe unit tests

* remove coverage files

* remove accidental diff

* remove accidental diff again

* fix linting

* remove testing aliases

* make calculate threshold error global

* add go generate comment for babe state TODO/make sure ok form

* fix mock generation

* add newline to end of build integration test

* newlines

* wip/newline

* test line endings

* wip

* remove crypto integration test

* change threshold err message

* wip

* test

* remove test comments

* clean up

* fix build error

* CR feedback

* more CR feedback

* change threshold error name

* remove empty fields

* change how mockgen used

* remove test log

* cr feedback

* remove errors from helper funcs

* final helper fix

* rebase and lint

* CR feedback

* lint

Co-authored-by: Eclésio Junior <eclesiomelo.1@gmail.com>
jimjbrettj pushed a commit that referenced this pull request Jan 18, 2022
@EclesioMeloJunior @jimjbrettj
chore(pkg/scale): return error if Result already has an assigned va…
3ef0299 
…lue (#2143)

* fix: return error if result already has an assigned value

* chore: include unit test

* chore: fix typo `ErrResultAlreadySet`

* chore: remove unneeded `require.Error`

* chore: fix undeclared name

* chore: remove packaged scope var to avoid problems with result type

* chore: fix result.Set error at offchain test
jimjbrettj pushed a commit that referenced this pull request Jan 18, 2022
@EclesioMeloJunior @jimjbrettj
chore(pkg/scale): return error if Result already has an assigned va…
314b0a1 
…lue (#2143)

* fix: return error if result already has an assigned value

* chore: include unit test

* chore: fix typo `ErrResultAlreadySet`

* chore: remove unneeded `require.Error`

* chore: fix undeclared name

* chore: remove packaged scope var to avoid problems with result type

* chore: fix result.Set error at offchain test
jimjbrettj added a commit that referenced this pull request Jan 21, 2022
@jimjbrettj @EclesioMeloJunior
testing (dot/core): rewrite message.go unit tests (#2197)
d800e07 
* chore(pkg/scale): return error if `Result` already has an assigned value  (#2143)

* fix: return error if result already has an assigned value

* chore: include unit test

* chore: fix typo `ErrResultAlreadySet`

* chore: remove unneeded `require.Error`

* chore: fix undeclared name

* chore: remove packaged scope var to avoid problems with result type

* chore: fix result.Set error at offchain test

* test txt count and generate mocks with mockgen

* WIP/hanldeTxnMsgTest

* WIP/core message tests

* wip/finish core message tests

* wip/message test

* fix reporting issue

* test core messages

* remove comments and lint

* remove unused file

* wip/cr feedback

* use dummy error for tests

* wip/finish cr feedback

* wip/move message validation to a separate function

* move txn validity check to new func

* lint

* fix variable naming to make less confusing

* remove pointer from validateTxn helper params

* refactor tests to define mocks in subtest

* CR feedback

* finish feedback for core tests

* define runtime mocks in subtest

* remane validTxn var

Co-authored-by: Eclésio Junior <eclesiomelo.1@gmail.com>
jimjbrettj added a commit that referenced this pull request Jan 21, 2022
@jimjbrettj @EclesioMeloJunior
testing (core): move existing tests to integration tests, rewrite `me…
0a25d1a 
…ssage.go` unit tests (#2224)

* chore(pkg/scale): return error if `Result` already has an assigned value  (#2143)

* fix: return error if result already has an assigned value

* chore: include unit test

* chore: fix typo `ErrResultAlreadySet`

* chore: remove unneeded `require.Error`

* chore: fix undeclared name

* chore: remove packaged scope var to avoid problems with result type

* chore: fix result.Set error at offchain test

* migrate core tests to integration tests

* remove test.log

* upgrade integration tests to match dev

* testing (dot/core): rewrite message.go unit tests (#2197)

* chore(pkg/scale): return error if `Result` already has an assigned value  (#2143)

* fix: return error if result already has an assigned value

* chore: include unit test

* chore: fix typo `ErrResultAlreadySet`

* chore: remove unneeded `require.Error`

* chore: fix undeclared name

* chore: remove packaged scope var to avoid problems with result type

* chore: fix result.Set error at offchain test

* test txt count and generate mocks with mockgen

* WIP/hanldeTxnMsgTest

* WIP/core message tests

* wip/finish core message tests

* wip/message test

* fix reporting issue

* test core messages

* remove comments and lint

* remove unused file

* wip/cr feedback

* use dummy error for tests

* wip/finish cr feedback

* wip/move message validation to a separate function

* move txn validity check to new func

* lint

* fix variable naming to make less confusing

* remove pointer from validateTxn helper params

* refactor tests to define mocks in subtest

* CR feedback

* finish feedback for core tests

* define runtime mocks in subtest

* remane validTxn var

Co-authored-by: Eclésio Junior <eclesiomelo.1@gmail.com>

* CR feedback

* finish feedback

* CR feedback

Co-authored-by: Eclésio Junior <eclesiomelo.1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimjbrettj jimjbrettj jimjbrettj approved these changes

@qdm12 qdm12 qdm12 approved these changes

@edwardmack edwardmack Awaiting requested review from edwardmack

@kishansagathiya kishansagathiya Awaiting requested review from kishansagathiya kishansagathiya is a code owner

@noot noot Awaiting requested review from noot

@timwu20 timwu20 Awaiting requested review from timwu20 timwu20 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

scale.Result cannot allow more then one Set call
3 participants
@EclesioMeloJunior @jimjbrettj @qdm12