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

[Testing] chore: add testkeyring pkg #237

Merged
merged 7 commits into from
Dec 5, 2023

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Nov 30, 2023

Summary

Human Summary

Adds a set of pre-generated accounts (i.e. addresses and mnemonics) and APIs which can be used to consume them. This is useful for populating the keyring and/or module genesis states.

It also includes the package used to generate the accounts and adds a make target for its usage (go_testgen_accounts).

AI Summary

Summary generated by Reviewpad on 04 Dec 23 11:51 UTC

This pull request includes the following changes:

  1. In the file miner_test.go, the comment regarding regenerating fixtures has been updated. The previous comment mentioned using "make go_fixturegen", while the updated comment suggests using "make go_testgen_fixtures" to regenerate fixtures.

  2. In the file template.go in the pkg/relayer/miner/gen/ directory, there are changes to comments regarding regenerating fixtures. The comment now recommends using the command make go_testgen_fixtures to regenerate all fixtures and go generate ./pkg/relayer/miner/miner_test.go to regenerate only this test's fixtures.

  3. In the file accounts.go in the testutil/testkeyring package, a new file has been introduced. It defines several structs and functions related to pre-generated accounts used in tests. The file provides a way to iterate over pre-generated accounts, retrieve accounts at specific indexes, and encode/decode account information using base64 and JSON.

  4. The diff in the file relay_fixtures_test.go in the package pkg/relayer/miner includes changes to generated test fixtures. It is advised not to manually edit this file as any changes will be overwritten upon regeneration. If you need to regenerate all fixtures, use the command make go_testgen_fixture. If you only want to regenerate fixtures for this specific test, run go generate ./pkg/relayer/miner/miner_test.go.

  5. There are changes made to the file testkeyring/keyring.go. It includes the addition of a new file with changes related to the creation and management of pre-generated accounts in a keyring for testing purposes.

  6. A new file testutil/testkeyring/gen_accounts/gen.go has been added. It is a Go program that generates pre-generated accounts. It reads command-line flags, creates a new in-memory keyring, generates accounts, and stores their information in a file.

Please review and verify these changes.

Issue

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Run all unit tests: make go_develop_and_test
  • Verify Localnet manually: See the instructions [here](TODO: add link to instructions)

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have performed a self-review of my own code
  • I have commented my code, updated documentation and left TODOs throughout the codebase

@bryanchriswhite bryanchriswhite added enhancement New feature or request testing Test (or test utils) additions, fixes, improvements or other labels Nov 30, 2023
@bryanchriswhite bryanchriswhite self-assigned this Nov 30, 2023
Makefile Outdated Show resolved Hide resolved
testutil/testkeyring/accounts_table.go Show resolved Hide resolved
testutil/testkeyring/accounts_table.go Outdated Show resolved Hide resolved
testutil/testkeyring/accounts.go Outdated Show resolved Hide resolved
testutil/testkeyring/keyring.go Show resolved Hide resolved
testutil/testkeyring/accounts.go Outdated Show resolved Hide resolved
testutil/testkeyring/accounts.go Outdated Show resolved Hide resolved
testutil/testkeyring/accounts.go Outdated Show resolved Hide resolved

// Next returns the next account in the iterator. It is safe to call
// concurrently and is guaranteed to return a unique account on each call.
func (iter *PreGeneratedAccountIterator) Next() (*PreGeneratedAccount, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I really like this design but wanted to challenge (again) the need for an iterator at all
  2. I came by [1], and WHAT A THREAD

[1] golang/go#54245

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤩 Nice find - thanks for sharing!

I definitely want to iterate on he current design but I think an iterator pattern will support much simpler and easier to reason about tests/helpers. My goal was to still support access by index, as in some specific cases that makes more sense; however, I imagine that the majority of the time it will just be more convenient to call a #Next() method to get a unique account instead of having to track an index and/or a container yourself. I also plan to add some examples to illustrate what I'm imagining.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely want to iterate on he current design but I think an iterator pattern will support much simpler and easier to reason about tests/helpers. My goal was to still support access by index, as in some specific cases that makes more sense; however, I imagine that the majority of the time it will just be more convenient to call a #Next() method to get a unique account instead of having to track an index and/or a container yourself. I also plan to add some examples to illustrate what I'm imagining.

I understand where this is coming from but still feel like we're overengineering something that can be a basic list. I'm not 100% convinced we'll need this at the moment, so will push back unless unless using #Next() makes the first application of this pattern easier to reason about.

Copy link
Contributor Author

@bryanchriswhite bryanchriswhite Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that it already does in #236. The fact that the keyring can be populated in one line completely separately from where the keys purposes are designated and used in on-chain state is indicative of the kind of coordination avoidance this enables. I feel like we would end up re-implementing this behavior in several places if we don't centralize it here, it seems inevitable to me that we would be iterating over such a list.

Copy link
Contributor Author

@bryanchriswhite bryanchriswhite Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requirements here will continue to get more complex (at least) until we finish with proof validation. Wdyt about waiting until then before rendering final judgement?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wdyt about waiting until then before rendering final judgement?

I personally think that if we merge it in, we'll never go back to remove it.

However, you seem pretty adamant that this is he right approach so I trust you on that 🤝

testutil/testkeyring/accounts.go Outdated Show resolved Hide resolved
@bryanchriswhite bryanchriswhite force-pushed the issues/140/chore/testkeyring branch from 318e3f4 to ff09bb1 Compare December 4, 2023 11:46
bryanchriswhite and others added 3 commits December 4, 2023 12:50
…yring

* pokt/main:
  chore: correct links to blog posts in README.md (#222)
@bryanchriswhite bryanchriswhite force-pushed the issues/140/chore/testkeyring branch from ff09bb1 to 9d5f62b Compare December 4, 2023 11:51
@bryanchriswhite bryanchriswhite marked this pull request as ready for review December 4, 2023 11:52
Name: name[0],
Address: pga.Address,
// Marshal returns the base64 and JSON encoded account string.
func (pga *PreGeneratedAccount) Marshal() (string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, ty for doing this!

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved!!

@bryanchriswhite bryanchriswhite merged commit 7f1c5af into main Dec 5, 2023
9 checks passed
bryanchriswhite added a commit that referenced this pull request Dec 5, 2023
…/polylog-zerolog

* issues/181/test/polylog-testutils:
  chore: rename `FnMethodSpy` -> `EventFuncSpy`
  [Testing] chore: add testkeyring pkg (#237)
bryanchriswhite added a commit that referenced this pull request Dec 5, 2023
…olylog-context

* issues/181/feat/polylog-zerolog: (21 commits)
  refactor: rename `FnMethodSpy` -> `EventFuncSpy`
  [Off-chain, Polylog] test: add testutils (#209)
  chore: rename `FnMethodSpy` -> `EventFuncSpy`
  [Testing] chore: add testkeyring pkg (#237)
  chore: update go.mod
  chore: add event method names to test cases
  chore: chore: review feedback improvements
  chore: review feedback improvements
  fix: test
  fix: goimports
  chore: review feedback improvements
  chore: review feedback improvements
  chore: correct links to blog posts in README.md (#222)
  chore: review feedback improvements
  chore: review feedback improvements
  [Rings] Create a shared `RingCache` package (#187)
  bump rollkit/cosmos-sdk & celestia node (#231)
  [E2E Relay, Configs] Fix localnet E2E relay issue - Ready for Tilt Integration (#225)
  [CI/LocalNet/E2E] Resolve issues with E2E tests (#198)
  fix: Makefile: `todo_this_commit` target & remove redundant `TODO_KEYWORDS` (#216)
  ...
bryanchriswhite added a commit that referenced this pull request Dec 5, 2023
…or/off-chain-logging

* issues/181/feat/polylog-context: (24 commits)
  chore: review feedback improvements
  chore: review feedback improvements
  [Off-chain, Polylog] chore: add zerolog logger implementation (#211)
  refactor: rename `FnMethodSpy` -> `EventFuncSpy`
  [Off-chain, Polylog] test: add testutils (#209)
  chore: rename `FnMethodSpy` -> `EventFuncSpy`
  [Testing] chore: add testkeyring pkg (#237)
  chore: update go.mod
  chore: add event method names to test cases
  chore: chore: review feedback improvements
  chore: review feedback improvements
  fix: test
  fix: goimports
  chore: review feedback improvements
  chore: review feedback improvements
  chore: correct links to blog posts in README.md (#222)
  chore: review feedback improvements
  chore: review feedback improvements
  [Rings] Create a shared `RingCache` package (#187)
  bump rollkit/cosmos-sdk & celestia node (#231)
  ...
bryanchriswhite added a commit that referenced this pull request Dec 6, 2023
…yring

* pokt/main:
  [Off-chain, Polylog] refactor: replace std logger usage in pkg/... with polylog (#219)
  [Off-chain, Polylog] feat: polylog context integration (#218)
  [Off-chain, Polylog] chore: add zerolog logger implementation (#211)
  [Off-chain, Polylog] test: add testutils (#209)
  [Testing] chore: add testkeyring pkg (#237)
bryanchriswhite added a commit that referenced this pull request Dec 6, 2023
…im-msg-validation

* issues/140/chore/testkeyring:
  [Off-chain, Polylog] refactor: replace std logger usage in pkg/... with polylog (#219)
  [Off-chain, Polylog] feat: polylog context integration (#218)
  [Off-chain, Polylog] chore: add zerolog logger implementation (#211)
  [Off-chain, Polylog] test: add testutils (#209)
  [Testing] chore: add testkeyring pkg (#237)
okdas pushed a commit that referenced this pull request Nov 14, 2024
* chore: add testkeyring pkg

* chore: self-review improvements

* chore: review feedback improvements

* chore: review feedback improvements

Co-authored-by: Daniel Olshansky <[email protected]>

* wip

* chore: chore: review feedback improvements

Co-authored-by: Daniel Olshansky <[email protected]>

---------

Co-authored-by: Daniel Olshansky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request testing Test (or test utils) additions, fixes, improvements or other
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants