prefetcher: improve error codes #3815
Conversation
ledger/ledgercore/error.go
Outdated
| } | ||
|
|
||
| // GroupTaskError indicates the group index of the unfulfilled resource | ||
| type GroupTaskError struct { |
There was a problem hiding this comment.
two requests:
- move the GroupTaskError to the prefetcher package.
- Make GroupTaskError be used with &GroupTaskError, and have the
Unwrap() errorimplemented, so that the caller could use theerrors.Isanderrors.As.
| func (l *prefetcherTestLedger) LookupWithoutRewards(_ basics.Round, addr basics.Address) (ledgercore.AccountData, basics.Round, error) { | ||
| func (l *prefetcherTestLedger) LookupWithoutRewards(rnd basics.Round, addr basics.Address) (ledgercore.AccountData, basics.Round, error) { | ||
| if _, has := l.errorAddress[addr]; has { | ||
| return ledgercore.AccountData{}, l.round, fmt.Errorf("Lookup Error") |
There was a problem hiding this comment.
you might want to use a custom error type here so that you could make errors.Is when testing.
| return ledgercore.AppResource{}, nil | ||
| } | ||
| func (l *prefetcherTestLedger) LookupAsset(rnd basics.Round, addr basics.Address, aidx basics.AssetIndex) (ledgercore.AssetResource, error) { | ||
| if aidx > 1000000 { |
There was a problem hiding this comment.
instead of using a threshold, make a given asset index invalid ( i.e. const errorTriggeringAssetIndex = 1000000 )
|
|
||
| // Test for the GroupIdx in the returned error | ||
| tc := testCases[3] | ||
| t.Run(tc.name, func(t *testing.T) { |
There was a problem hiding this comment.
if you're repeating the same test case name, please have this implemented in a separate Test function.
| // if there is an error, report the error to the output channel. | ||
| p.outChan <- LoadedTransactionGroup{ | ||
| Err: done.err, | ||
| Err: ledgercore.GroupTaskError{Err: done.err, GroupIdx: done.groupIdx}, |
There was a problem hiding this comment.
With your change, the Err is always either of type ledgercore.GroupTaskError or nil. It sounds as if you should change the Err datatype to be *GroupTaskError.
| tc := testCases[3] | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| errorReceived := false | ||
| groups := make([][]transactions.SignedTxnWithAD, 5) |
There was a problem hiding this comment.
I think that this test would be much more readable if you'll use explicit use cases ( as above ), instead of programmatically modifying the test cases.
Codecov Report
@@ Coverage Diff @@
## master #3815 +/- ##
==========================================
+ Coverage 49.99% 50.01% +0.02%
==========================================
Files 392 393 +1
Lines 68495 68503 +8
==========================================
+ Hits 34242 34265 +23
+ Misses 30497 30487 -10
+ Partials 3756 3751 -5
Continue to review full report at Codecov.
|
4 participants
Enhance the error reporting in prefetcher
resolves https://github.com/algorand/go-algorand-internal/issues/1921