Initial signature aggregator unit tests #457
Conversation
feuGeneA
force-pushed
the
sig-agg-unit-tests
branch
from
2c3ca1b
to
77cf6d0
Compare
feuGeneA
force-pushed
the
sig-agg-unit-tests
branch
from
77cf6d0
to
e586c92
Compare
feuGeneA
changed the title
feuGeneA
requested review from
iansuvak,
geoff-vball,
minghinmatthewlam,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
| @@ -343,6 +343,8 @@ func (s *SignatureAggregator) CreateSignedMessage( | |||
| return nil, errNotEnoughSignatures | |||
| } | |||
|
|
|||
| // TODO: consider making this function private. its only reference seems to be | |||
There was a problem hiding this comment.
Yup both of the below should be private. That was my oversight in the move
There was a problem hiding this comment.
addressed in e5d44b4
addresses review comment #457 (comment)
| @@ -1,6 +1,8 @@ | |||
| // Copyright (C) 2023, Ava Labs, Inc. All rights reserved. | |||
| // See the file LICENSE for licensing terms. | |||
|
|
|||
| //go:generate mockgen -source=$GOFILE -destination=./mocks/mock_app_request_network.go -package=mocks | |||
There was a problem hiding this comment.
why is this included in the source file? I would expect the source file to not need to have knowledge of the mock file at all. Instead, can the mock file keep track of command needed to regenerate?
There was a problem hiding this comment.
it's here because the mock is generated from this source file, which is represented on this line by $GOFILE. btw i took this from your example 😅 at https://github.com/ava-labs/awm-relayer/pull/21/files#diff-b7c4d6477bfb555408745965840c387dcfc91a74bcbdc3b30d31d594e9abc0e5R4
my understanding is that the mock source file is 100% generated so we can't really put anything in there.
There was a problem hiding this comment.
Generally LGTM but I do prefer Mock Checker that @minghinmatthewlam suggested as the name visible on the UI rather than the more verbose one, it also follows the pattern of ABI Go Bindings Checker.
scripts/build.sh
Outdated
| # Root directory | ||
| root=$( | ||
| cd "$(dirname "${BASH_SOURCE[0]}")" | ||
| cd .. && pwd | ||
| ) | ||
|
|
||
| "$root"/scripts/generate.sh |
There was a problem hiding this comment.
I prefer this not being included in the build script to make it run faster if iterating on some manual testing. That being said in that case you would probably be running a specific build script as well so feel free to drop if you disagree
Co-authored-by: minghinmatthewlam <matthew.lam@avalabs.org> Signed-off-by: F. Eugene Aumson <feuGeneA@users.noreply.github.com>
addresses review comment #457 (comment)
addresses review comment #457 (comment)
addresses review comment #457 (comment)
feuGeneA
force-pushed
the
sig-agg-unit-tests
branch
from
eb2e83e
to
9396b4b
Compare
addresses review comment #457 (comment)
|
|
||
| go build -o tests/cmd/decider/decider ./tests/cmd/decider/ | ||
|
|
||
| "$root"/scripts/generate.sh |
There was a problem hiding this comment.
I would still rather call this manually than have it be re-generated automatically ever. If it's quick it might not make too much of a difference. not a blocker though
There was a problem hiding this comment.
I agree. I wouldn't ever expect running a test to alter my git status
addresses review comment #457 (comment)
addresses review comment #457 (comment)
addresses review comment #457 (comment)
addresses review comment #457 (comment)
addresses review comment #457 (comment)
Why this should be merged
it's a first step towards fixing #446