-
Notifications
You must be signed in to change notification settings - Fork 278
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.
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
refactor: consolidate duplicate test methods #1074
refactor: consolidate duplicate test methods #1074
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1074 +/- ##
==========================================
- Coverage 50.50% 50.42% -0.08%
==========================================
Files 71 72 +1
Lines 4390 4387 -3
==========================================
- Hits 2217 2212 -5
- Misses 1946 1947 +1
- Partials 227 228 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vidhanarya for this stellar PR! I'm really happy to see this net negative lines of code diff
None of my feedback is blocking this PR from being merged. Let's wait for @evan-forbes to review before merging 🚀
testutil/testfactory/types.go
Outdated
@@ -0,0 +1,62 @@ | |||
package testfactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional] since this file doesn't define any new types, proposal to rename it from types.go
to something else?
Proposals:
generate.go
generate_tx.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, the idea here was that any file inside factory testfactory/[package_for_which_factory_is_needed].go
represents the package from where definitions are being used (in this case Blob
and Txs
from tendermint's types
package.
But if convention is different here then generate_tx.go
or just tx.go
(in this case, apt thing to do would be to move blob generator methods to another file blob.go
) also work. Changing to generate_tx.go
for now, can update if any better suggestion comes up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining the rationale. Given that rationale, I'm comfortable with the name: testutil/testfactory/types.go
. I'm also comfortable with splitting the generation into generate_tx.go
and generate_blob.go
so defer to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, changing to for now, will also wait for @evan-forbes if he have any input here since I saw a similar structure being used in generate_txs.go
and generate_blob.go
celestia-core
(https://github.com/celestiaorg/celestia-core/blob/v0.34.x-celestia/test/factory/tx.go#L1)
EDIT: Changing to txs.go
and blob.go
testutil/testfactory/types.go
Outdated
|
||
func GenerateRandomBlob(size int) types.Blob { | ||
blob := types.Blob{ | ||
NamespaceID: tmrand.Bytes(appconsts.NamespaceSize), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[no change needed] in the future, we may want this to generate a valid blob namespace. In other words, something like https://github.com/celestiaorg/celestia-app/blob/5bbdac2d3f46662a34b2111602b8f964d6e6fba5/testutil/namespace/random_ns_generator.go#L11-L23
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really great work @vidhanarya !! sincerely, thank you for getting rid of all that crap!!
we just merged a rather large naming PR, which caused a bunch of conflicts, so unfortunately we have to fix those, but then we can merge this!
thanks again 💯 🙂
This is done. @evan-forbes @rootulp Please have a final look to confirm |
14a815d
to
f48d236
Compare
…edBlob(s) in testutil
f48d236
to
fbca36f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 💯 🤸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great work @vidhanarya 💯
} | ||
|
||
// SetupWithGenesisValSet initializes GenesisState with a single validator and genesis accounts | ||
// GenesisStateWithSingleValidator initializes GenesisState with a single validator and genesis accounts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[no change needed] it would be awesome if we enabled a lint rule that flagged when the godoc for a function doesn't start with the name of the function. In the meantime, this type of correction is def appreciated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this would be a nice addition 💯
Overview
Closes #1073
Affected packages
pkg
->shares
&prove
generateRandomTransaction
,generateRandomlySizedTransactions
,generateRandomBlob
, andgenerateRandomlySizedBlobs
testutil
factory
package with generator method for the random transaction(s) and random blob(s)GenerateKeyringSigner
andgenerateKeyring
methodsx/blob/types
test_util.go
x/blob/types
inapp
packageapp
/app_test
app
package'stest_util.go
blobtypes
Checklist
New and updated code has appropriate documentationVisual proof for any user facing features like CLI or documentation updates