Wrap errors returned from Store by mauriceLC92 · Pull Request #404 · evstack/ev-node

mauriceLC92 · 2022-05-14T13:25:46Z

Wraps errors returned from the Store

adlerjohn

CI failing

mauriceLC92 · 2022-05-15T17:33:33Z

Hmm I wonder if another flakey test? Ran the tests locally before opening PR and was fine. Also these failures look unrelated to the changes being added @tzdybal

tzdybal

The tests are failing consistently locally. I left comments.

tzdybal · 2022-05-15T18:00:03Z

 	commit := new(types.Commit)
 	err = commit.UnmarshalBinary(commitData)
-	return commit, err
+	return commit, fmt.Errorf("error decoding binary data of Commit into object: %w", err)


Gotcha! In original case, if error was nil, nil was returned. Now, you're wrapping everytime, even if error is nil.

Suggested change

return commit, fmt.Errorf("error decoding binary data of Commit into object: %w", err)

if err != nil {

return nil, fmt.Errorf("error decoding binary data of Commit into object: %w", err)

}

return commit, nil

Updated 👍

tzdybal · 2022-05-15T18:00:44Z

 	err = json.Unmarshal(blob, &state)
 	atomic.StoreUint64(&s.height, uint64(state.LastBlockHeight))
-	return state, err
+	return state, fmt.Errorf("error unmarshalling state from JSON encoding: %w", err)


Same here ;)

Updated 👍

mauriceLC92 · 2022-05-15T18:03:30Z

The tests are failing consistently locally. I left comments.

Thank you! I realized I was running the test locally on main 😂 - Now failing for me too. Will address

codecov-commenter · 2022-05-15T18:26:53Z

Codecov Report

Merging #404 (a030778) into main (644a85a) will decrease coverage by 0.06%.
The diff coverage is 25.92%.

@@            Coverage Diff             @@
##             main     #404      +/-   ##
==========================================
- Coverage   56.54%   56.47%   -0.07%     
==========================================
  Files          44       44              
  Lines        7518     7527       +9     
==========================================
  Hits         4251     4251              
- Misses       2659     2665       +6     
- Partials      608      611       +3

Impacted Files	Coverage Δ
store/store.go	`64.05% <25.92%> (-4.01%)`	⬇️

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 644a85a...a030778. Read the comment docs.

adlerjohn

utACK

tzdybal

Comments seems a bit incoherent (for example error encoding Block into binary form and error marshalling state into JSON encoding) - please stick to single form.

tzdybal · 2022-05-16T07:20:38Z

 		{"valid/no params", "/abci_info", http.StatusOK, -1, `"last_block_height":345`},
 		// to keep test simple, allow returning application error in following case
-		{"valid/int param", "/block?height=321", http.StatusOK, int(json2.E_INTERNAL), `"key not found"`},
+		{"valid/int param", "/block?height=321", http.StatusOK, int(json2.E_INTERNAL), "error loading hash by height 321: key not found"},


I think that it might not be "stable" to include block number here.

Suggested change

{"valid/int param", "/block?height=321", http.StatusOK, int(json2.E_INTERNAL), "error loading hash by height 321: key not found"},

{"valid/int param", "/block?height=321", http.StatusOK, int(json2.E_INTERNAL), "error loading block hash for height"},

tzdybal · 2022-05-16T07:20:56Z

 	var hash [32]byte
 	if err != nil {
-		return hash, err
+		return hash, fmt.Errorf("error loading hash by height %v: %w", height, err)


Suggested change

return hash, fmt.Errorf("error loading hash by height %v: %w", height, err)

return hash, fmt.Errorf("error loading block hash for height %v: %w", height, err)

Hey @tzdybal sorry I am a little unsure as to what you mean by the singular form in this case

tzdybal

Now it looks way better :)

tzdybal · 2022-07-06T09:56:59Z

 	blob, err := s.db.Get(getStateKey())
 	if err != nil {
-		return types.State{}, err
+		return types.State{}, fmt.Errorf("failed to retrieve key: %w", err)


Suggested change

return types.State{}, fmt.Errorf("failed to retrieve key: %w", err)

return types.State{}, fmt.Errorf("failed to retrieve state: %w", err)

tzdybal · 2022-07-06T10:22:04Z

 		{"valid/no params", "/abci_info", http.StatusOK, -1, `"last_block_height":"345"`},
 		// to keep test simple, allow returning application error in following case
-		{"valid/int param", "/block?height=321", http.StatusOK, int(json2.E_INTERNAL), `"key not found"`},
+		{"valid/int param", "/block?height=321", http.StatusOK, int(json2.E_INTERNAL), "error loading block hash for height"},


This won't pass.

mauriceLC92 added 3 commits March 26, 2022 09:59

wrapped errors in the store.go file

275becb

Merge branch 'main' into mauriceLC92/wrap-errors-from-store

e18bfbc

updates error to be descriptive for LoadState

858cef9

adlerjohn reviewed May 14, 2022

View reviewed changes

adlerjohn assigned mauriceLC92 May 14, 2022

adlerjohn linked an issue May 14, 2022 that may be closed by this pull request

Wrap errors returned from Store #288

Closed

adlerjohn added the T:enhancement label May 14, 2022

mauriceLC92 marked this pull request as ready for review May 15, 2022 17:34

mauriceLC92 requested review from liamsi and tzdybal as code owners May 15, 2022 17:34

tzdybal suggested changes May 15, 2022

View reviewed changes

Updated based on PR comments - fixes the failing tests

a030778

tzdybal requested review from adlerjohn and tzdybal May 15, 2022 18:50

adlerjohn approved these changes May 15, 2022

View reviewed changes

tzdybal reviewed May 16, 2022

View reviewed changes

Merge branch 'main' into mauriceLC92/wrap-errors-from-store

63c0d52

adlerjohn changed the title ~~Resolve - Wrap errors returned from Store~~ Wrap errors returned from Store May 19, 2022

mauriceLC92 added 3 commits July 6, 2022 06:58

updates error string based on PR comments

df85882

Merge branch 'main' into mauriceLC92/wrap-errors-from-store

7ec100b

add formatted error to LoadState

da1acdf

tzdybal reviewed Jul 6, 2022

View reviewed changes

updated LoadState error string

678130f

tzdybal approved these changes Jul 7, 2022

View reviewed changes

tzdybal merged commit 8cd8832 into evstack:main Jul 7, 2022

	{"valid/int param", "/block?height=321", http.StatusOK, int(json2.E_INTERNAL), "error loading hash by height 321: key not found"},
	{"valid/int param", "/block?height=321", http.StatusOK, int(json2.E_INTERNAL), "error loading block hash for height"},

	return hash, fmt.Errorf("error loading hash by height %v: %w", height, err)
	return hash, fmt.Errorf("error loading block hash for height %v: %w", height, err)

	return types.State{}, fmt.Errorf("failed to retrieve key: %w", err)
	return types.State{}, fmt.Errorf("failed to retrieve state: %w", err)

Conversation

mauriceLC92 commented May 14, 2022

Uh oh!

adlerjohn left a comment

Choose a reason for hiding this comment

Uh oh!

mauriceLC92 commented May 15, 2022

Uh oh!

tzdybal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mauriceLC92 commented May 15, 2022

Uh oh!

codecov-commenter commented May 15, 2022

Codecov Report

Uh oh!

adlerjohn left a comment

Choose a reason for hiding this comment

Uh oh!

tzdybal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tzdybal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants