Skip to content
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

Merge duplicate definitions with testutil #1073

Closed
vidhanarya opened this issue Nov 26, 2022 · 2 comments · Fixed by #1074
Closed

Merge duplicate definitions with testutil #1073

vidhanarya opened this issue Nov 26, 2022 · 2 comments · Fixed by #1074
Assignees
Labels
testing items that are strictly related to adding or extending test coverage

Comments

@vidhanarya
Copy link
Contributor

vidhanarya commented Nov 26, 2022

Subtask for #832

Found duplicated function definitions for generateRandomlySizedTransactions and generateRandomlySizedBlobs
Example ref:
generateRandomlySizedTransaction(s)

func generateRandomlySizedTransactions(count, maxSize int) coretypes.Txs {

func generateRandomTransaction(count, size int) coretypes.Txs {

func generateRandomlySizedTxs(count, max int) types.Txs {

func generateRandomTxs(count, size int) types.Txs {

Identified Duplicates:

Duplicates Packages
generateRandomlySizedTransactions,generateRandomlySizedTxs shares,prove
generateRandomTransaction, generateRandomTxs shares,prove
generateRandomlySizedBlobs shares,prove
generateRandomBlob shares,prove
GenerateKeyringSigner, generateKeyringSigner, generateKeyring app, app_test, testutil, x/blob/types
GenerateManyRawWirePFB, GenerateManyRawSendTxs, generateRawTx, generateManyRawWirePFB, generateManyRawSendTxs app, app_test, blobtestutil

Proposal:

  • Create a factory package inside testutil and move all these generators to factory package
@vidhanarya
Copy link
Contributor Author

vidhanarya commented Nov 26, 2022

@evan-forbes @rootulp let me know if this proposal works. I have started working on the changes and can raise a PR if no concerns.

Also, couldn't find any epic to link this issue, can someone point me to that if any?
Reason: Have a few concerns/proposals (all comes under the testing label) regarding the domain of other packages as well - for example:

  1. test_util.go in the app package collides with the domain of testutil package
  2. Constant declarations of common variables for testing purposes in individual test files (eg. testAccName in builder_test.go) instead of a single source in testutil (eg. test_constants.go)

Ref:

  1. testAccName = "test-account"

  2. Not in use?

    func GenerateValidBlockData(

  3. Duplicates

    func generateManyRawWirePFB(t *testing.T, txConfig client.TxConfig, signer *types.KeyringSigner, count, size int) [][]byte {

    func GenerateManyRawWirePFB(t *testing.T, txConfig client.TxConfig, signer *types.KeyringSigner, count, size int) [][]byte {

@rootulp rootulp added the testing items that are strictly related to adding or extending test coverage label Nov 28, 2022
@rootulp
Copy link
Collaborator

rootulp commented Nov 28, 2022

Also, couldn't find any epic to link this issue, can someone point me to that if any?

I don't think this falls into an existing epic but we can def answer your other questions without an epic assigned.

test_util.go in the app package collides with the domain of testutil package

I get confused by the number of testutil things in this repo. I like your name proposal for a factory package that contains generate* functions.

Constant declarations of common variables for testing purposes in individual test files

I don't think there's a ton of value to be gained by consolidating constants used in test files but I'm not against it. For a bit of context, a PR author may create a new constant while writings tests for their PR because they're unaware that another constant may suffice.

I think there is value in consolidating all the generate* functions you've done in #1074 because now future PRs shouldn't need to copy + paste this code around for setting up data in tests.

Not in use?

If not in use, let's delete it 🪓 . CI should fail if it is needed.

@vidhanarya vidhanarya changed the title Remove duplicate definitions of generateRandomlySizedBlob(s) & generateRandomlySizedTransaction(s) Merge duplicate definitions with testutil Nov 28, 2022
evan-forbes pushed a commit that referenced this issue Nov 29, 2022
## Overview
Closes #1073

Affected packages
1. `pkg` -> `shares` & `prove`
- Remove duplicated `generateRandomTransaction`,
`generateRandomlySizedTransactions`, `generateRandomBlob`, and
`generateRandomlySizedBlobs`
2. `testutil`
- Add `factory` package with generator method for the random
transaction(s) and random blob(s)
    - Deprecate blobtestutil
- Remove duplicated `GenerateKeyringSigner` and `generateKeyring`
methods
3. `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
4.  `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

- [X] ~~New and updated code has appropriate documentation~~
- [X] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [X] ~~Visual proof for any user facing features like CLI or
documentation updates~~
- [X] Linked issues closed with keywords
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing items that are strictly related to adding or extending test coverage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants