refactor: consolidate duplicate test methods #1074

vidhanarya · 2022-11-26T16:32:19Z

Overview

Closes #1073

Affected packages

pkg -> shares & prove
- Remove duplicated generateRandomTransaction, generateRandomlySizedTransactions, generateRandomBlob, and generateRandomlySizedBlobs
testutil
- Add factory package with generator method for the random transaction(s) and random blob(s)
- Deprecate blobtestutil
- Remove duplicated GenerateKeyringSigner and generateKeyring methods
x/blob/types
- Refactor keyring generator method in separate file test_util.go
- Reuse keyring generators from x/blob/types in app package
- Use constants instead of hardcoded strings for the test account and chain id
app/app_test
- Refactor all helper generate methods in app package's test_util.go
- Reuse keyring generators from blobtypes
- Simplify some of the helper generateTxs methods

Checklist

~~New and updated code has appropriate documentation~~
New and updated code has new and/or updated testing
Required CI checks are passing
~~Visual proof for any user facing features like CLI or documentation updates~~
Linked issues closed with keywords

codecov-commenter · 2022-11-26T16:37:50Z

Codecov Report

Merging #1074 (fbca36f) into main (2f89956) will decrease coverage by 0.07%.
The diff coverage is 45.16%.

@@            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

Impacted Files	Coverage Δ
app/test_util.go	`71.84% <35.55%> (-2.14%)`	⬇️
x/blob/types/test_util.go	`70.58% <70.58%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rootulp

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 🚀

rootulp · 2022-11-28T16:33:26Z

testutil/testfactory/types.go

@@ -0,0 +1,62 @@
+package testfactory


[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

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.

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.

Cool, changing to ~~generate_txs.go and generate_blob.go~~ for now, will also wait for @evan-forbes if he have any input here since I saw a similar structure being used in 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

rootulp · 2022-11-28T16:37:05Z

testutil/testfactory/types.go

+
+func GenerateRandomBlob(size int) types.Blob {
+	blob := types.Blob{
+		NamespaceID:  tmrand.Bytes(appconsts.NamespaceSize),


[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

testutil/testfactory/types.go

evan-forbes

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 💯 🙂

vidhanarya · 2022-11-29T05:34:59Z

~~Marking as draft for now, will push an updated commit (with rename changes) and resolved merge conflicts by 6:30 PM UTC today.~~

This is done. @evan-forbes @rootulp Please have a final look to confirm message -> blob changes are accordingly migrated in testfactory/test_util changes. Also added one more commit to remove GenerateValidBlockData.

…edBlob(s) in testutil

…ckage

evan-forbes

👍 💯 🤸

rootulp

Really great work @vidhanarya 💯

rootulp · 2022-11-29T17:23:16Z

testutil/test_app.go

-}
-
-// SetupWithGenesisValSet initializes GenesisState with a single validator and genesis accounts
+// GenesisStateWithSingleValidator initializes GenesisState with a single validator and genesis accounts


[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!

Yeah, this would be a nice addition 💯

vidhanarya requested review from rootulp and evan-forbes as code owners November 26, 2022 16:32

rach-id assigned vidhanarya Nov 28, 2022

rootulp previously approved these changes Nov 28, 2022

View reviewed changes

rootulp mentioned this pull request Nov 28, 2022

Merge duplicate definitions with testutil #1073

Closed

vidhanarya dismissed rootulp’s stale review via 9aaf7a9 November 28, 2022 18:11

evan-forbes reviewed Nov 29, 2022

View reviewed changes

vidhanarya marked this pull request as draft November 29, 2022 05:33

vidhanarya force-pushed the consolidate-testutil branch from 14a815d to f48d236 Compare November 29, 2022 14:20

vidhanarya marked this pull request as ready for review November 29, 2022 14:27

vidhanarya added 12 commits November 29, 2022 19:57

chore: consolidate generateRandomTransaction(s) & generateRandomlySiz…

ac76dff

…edBlob(s) in testutil

refactor: minimise duplicates of test KeyringSigner generators

78d0b78

refactor: use test KeyringSigner generators from blob.types in app pa…

99477d1

…ckage

chore: rename factory to testfactory

e441f90

refactor: deprecate blobtestutil

f025c3f

refactor: test utils for app package

ef2eb39

chore: organize imports

b21fb4d

chore: rename GenerateRandomTransaction -> GenerateRandomTransactions

f9acce7

chore: rename testfactory/types.go -> testfactory/generate_txs.go

e4ffbb8

refactor: split testfactory/generate_txs.go in txs.go` and blob.go

f30429a

chore: rebase bug fixes

47ceb96

chore: remove unused GenerateValidBlockData from app package

fbca36f

vidhanarya force-pushed the consolidate-testutil branch from f48d236 to fbca36f Compare November 29, 2022 14:28

evan-forbes approved these changes Nov 29, 2022

View reviewed changes

rootulp approved these changes Nov 29, 2022

View reviewed changes

evan-forbes merged commit 3941003 into celestiaorg:main Nov 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: consolidate duplicate test methods #1074

refactor: consolidate duplicate test methods #1074

vidhanarya commented Nov 26, 2022 •

edited by rootulp

codecov-commenter commented Nov 26, 2022 •

edited

rootulp left a comment

rootulp Nov 28, 2022

vidhanarya Nov 28, 2022

rootulp Nov 28, 2022

vidhanarya Nov 28, 2022 •

edited

rootulp Nov 28, 2022

evan-forbes left a comment •

edited

vidhanarya commented Nov 29, 2022 •

edited

evan-forbes left a comment

rootulp left a comment

rootulp Nov 29, 2022

rootulp Nov 29, 2022

vidhanarya Nov 30, 2022

refactor: consolidate duplicate test methods #1074

refactor: consolidate duplicate test methods #1074

Conversation

vidhanarya commented Nov 26, 2022 • edited by rootulp

Overview

Checklist

codecov-commenter commented Nov 26, 2022 • edited

Codecov Report

rootulp left a comment

Choose a reason for hiding this comment

rootulp Nov 28, 2022

Choose a reason for hiding this comment

vidhanarya Nov 28, 2022

Choose a reason for hiding this comment

rootulp Nov 28, 2022

Choose a reason for hiding this comment

vidhanarya Nov 28, 2022 • edited

Choose a reason for hiding this comment

rootulp Nov 28, 2022

Choose a reason for hiding this comment

evan-forbes left a comment • edited

Choose a reason for hiding this comment

vidhanarya commented Nov 29, 2022 • edited

evan-forbes left a comment

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

rootulp Nov 29, 2022

Choose a reason for hiding this comment

rootulp Nov 29, 2022

Choose a reason for hiding this comment

vidhanarya Nov 30, 2022

Choose a reason for hiding this comment

vidhanarya commented Nov 26, 2022 •

edited by rootulp

codecov-commenter commented Nov 26, 2022 •

edited

vidhanarya Nov 28, 2022 •

edited

evan-forbes left a comment •

edited

vidhanarya commented Nov 29, 2022 •

edited