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

[TECHDEBT][UTILITY] Fix Unit Tests using testing.M #282

Merged
merged 23 commits into from
Oct 7, 2022
Merged

Conversation

Olshansk
Copy link
Member

@Olshansk Olshansk commented Oct 4, 2022

Description

Fix unit tests that exist on the main branch.

Origin Document

Fixes conversation in #259:

Screen Shot 2022-10-04 at 4 07 23 PM

Issue

Rather than the core Go documentation, I believe this article does a better job at explaining why we had broken tests:

Screen Shot 2022-10-04 at 4 12 38 PM

Before this change, even when a unit test failed, we still got a pass:

Screen Shot 2022-10-04 at 4 26 09 PM

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • Update test_utility target in the Makefile
  • Not ignoring the exit code of m.Run() in all the files where we use it (several modules)
  • Fix tests in the utility module:
    • Remove redundant tests (e.g. TestUtilityContext_UnstakesPausedBefore which was replaced by TestUtilityContext_UnstakePausedBefore)
    • Do not ignore m.Run exit code so broken tests are caught
    • Improve readability of some tests (whitespace)
    • Do not unnecessarily export helpers
  • Fix unit tests in the persistence module related to type casting

Testing

  • make develop_test
  • LocalNet w/ all of the steps outlined in the README

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

@Olshansk Olshansk added utility Utility specific changes code health Nice to have code improvement labels Oct 4, 2022
@Olshansk Olshansk self-assigned this Oct 4, 2022
@Olshansk Olshansk marked this pull request as ready for review October 4, 2022 23:28
@Olshansk Olshansk requested a review from a team as a code owner October 4, 2022 23:28
@Olshansk Olshansk changed the title [TECHDEBT][UTILITY] Fix Unit Tests [TECHDEBT][UTILITY] Fix Unit Tests using testing.M Oct 4, 2022
Copy link
Contributor

@andrewnguyen22 andrewnguyen22 left a comment

Choose a reason for hiding this comment

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

The base change of catching the error code LGTM

However, you introduce a TON of white space into the tests. This makes me think we need a style guide or something because (a) it seems unnecessary to me (b) possibly out of scope?

I also see you marked the issue/PR as Utility, but you did this for all modules. Please update the issue

Copy link
Contributor

@andrewnguyen22 andrewnguyen22 left a comment

Choose a reason for hiding this comment

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

The base change of catching the error code LGTM

However, you introduce a TON of white space into the tests. This makes me think we need a style guide or something because (a) it seems unnecessary to me (b) possibly out of scope?

I also see you marked the issue/PR as Utility, but you did this for all modules. Please update the issue

utility/test/actor_test.go Show resolved Hide resolved
utility/test/actor_test.go Show resolved Hide resolved
p2p/CHANGELOG.md Outdated Show resolved Hide resolved
@Olshansk Olshansk added p2p P2P specific changes consensus Consensus specific changes persistence Persistence specific changes labels Oct 5, 2022
@Olshansk
Copy link
Member Author

Olshansk commented Oct 5, 2022

@andrewnguyen22 I reverted whiteline changes.

The base change of catching the error code LGTM

Ty.

I also see you marked the issue/PR as Utility, but you did this for all modules. Please update the issue

Updated.

However, you introduce a TON of white space into the tests. This makes me think we need a style guide or something because (a) it seems unnecessary to me (b) possibly out of scope?

Agreed that the whiteline changes are out of scope. Reverted.

Generally, I've been introducing whitelines between different "logical components" of the test. I agree that this is ill-defined and will update a best practice doc once I can quantity it.

E.g.

			ctx := NewTestingUtilityContext(t, 0)

			actor := getFirstActor(t, ctx, actorType)
			addrBz, err := hex.DecodeString(actor.GetAddress())
			require.NoError(t, err)
			msgEditStake := &typesUtil.MessageEditStake{
				Address:   addrBz,
				Chains:    test_artifacts.DefaultChains,
				Amount:    test_artifacts.DefaultStakeAmountString,
				ActorType: actorType,
			}

			candidates, err := ctx.GetMessageEditStakeSignerCandidates(msgEditStake)
			require.NoError(t, err)
			require.Equal(t, len(candidates), 2, "unexpected number of candidates")
			require.Equal(t, actor.GetOutput(), hex.EncodeToString(candidates[0]), "incorrect output candidate")
			require.Equal(t, actor.GetAddress(), hex.EncodeToString(candidates[1]), "incorrect addr candidate")
			test_artifacts.CleanupTest(ctx)
            // We get the context and used that to get the actor we use for the remainder of the test
			ctx := NewTestingUtilityContext(t, 0)
			actor := getFirstActor(t, ctx, actorType)

            // We get the address and verify there's no error
			addrBz, err := hex.DecodeString(actor.GetAddress())
			require.NoError(t, err)

            // We prepare a test artifact
			msgEditStake := &typesUtil.MessageEditStake{
				Address:   addrBz,
				Chains:    test_artifacts.DefaultChains,
				Amount:    test_artifacts.DefaultStakeAmountString,
				ActorType: actorType,
			}

            // We retrieve candidates, validate there's no error and run test checks using that return value
			candidates, err := ctx.GetMessageEditStakeSignerCandidates(msgEditStake)
			require.NoError(t, err)
			require.Equal(t, len(candidates), 2, "unexpected number of candidates")
			require.Equal(t, actor.GetOutput(), hex.EncodeToString(candidates[0]), "incorrect output candidate")
			require.Equal(t, actor.GetAddress(), hex.EncodeToString(candidates[1]), "incorrect addr candidate")

            // Test cleanup is a separate independent issue
			test_artifacts.CleanupTest(ctx)

@Olshansk Olshansk merged commit 983f480 into main Oct 7, 2022
@Olshansk Olshansk deleted the fix_all_main_tests branch October 7, 2022 16:54
deblasis added a commit to deblasis/pocket that referenced this pull request Oct 12, 2022
It was causing test failures because of inconsistent Postgresql state
deblasis added a commit that referenced this pull request Oct 13, 2022
## Description

This PR is a way to convey a design idea for the refactoring of the way we handle `config` and `genesis` as highlighted in a couple of Protocol Hours and also summarized, along with more context that should help us making the right decisions, into the discussion #234

## Issue

There's no issue per-se, this is basically TECHDEBT handling I guess. Let me know if I should create one.

## Type of change

Please mark the options that are relevant.

- [x] New feature, functionality or library
- [x] Code health or cleanup

## List of changes

- Abstracted config and genesis handling
- Mockable runtime
- Refactored all modules to use RuntimeMgr
- Updated RuntimeMgr to manage clock as well
- Modules now accept `interfaces` instead of paths.
- Unmarshalling is done in a new `runtime` package (runtime because what we do in there affects the runtime of the application)
- We are now able to accept configuration via environment variables (thanks to @okdas for inspiration and [sp13 for Viper]("github.com/spf13/viper"))


## Testing

- [x] `make test_all`
- [x] [LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md) w/ all of the steps outlined in the `README`

## Checklist

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have tested my changes using the available tooling
- [x] If applicable, I have made corresponding changes to related local or global README
- [x] If applicable, I have added new diagrams using [mermaid.js](https://mermaid-js.github.io)
- [x] If applicable, I have added tests that prove my fix is effective or that my feature works

* refactor(Config): runtime module

* style(Client): consistency

* fix(Config): make configPath and genesisPath available in Base

* style(Persistence): Persistance -> Persistence

* fix(Config): Base init

* feat(Config): viper config parsing + environment variables

* fix(Config): filename handling

* refactor(Config): runtime module and InitializableModule interface

* feat(Config): useRandomPK in runtime

* refactor(Config): renamed Builder / updated signatures

* style(Config): builder -> runtime

* feat(Config): implement InitlializableModule in all modules

* fix(Consensus): remove pacemakerConfig from interface

* feat(Config): params/flags injection (stab @Private key for CLI)

* style(Client): cleanup

* feat(Shared): ConfigurableModule and GenesisDependentModule 👀

* chore(go.mod): tidy

* feat(Shared): KeyholderModule 👀

* fix(Shared): Enforcing  for all modules

* refactor(Runtime): renamed b to rc

* style(Shared): renamed moduleCfg to moduleNameCfg

* fix(SHARED): build error for importing unnecessary package

* Update shared/modules/runtime_module.go

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

* refactor(runtime): renamed RuntimeConfig -> runtimeConfig

* fix(runtime): improved WithRandomPK()

* docs(runtime): // RuntimeConfig option helpers comment

* refactor(Shared): nodeModule, P2PAddressableModule

* refactor(Consensus): interface embedding

* refactor(Consensus): consistency in naming

* refactor(Persistence): consistence in naming

* feat(P2P): P2P module now implements ConfigurableModule

* fix(Persistence): improved DEBUG_CLEAR_STATE

* feat(Shared): modules implement ConfigurableModule

* refactor(Shared): consistency: adding config *structType in modules

* Update consensus/module.go

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

* fix(Runtime): fix WithPK for when called with a nil P2PConfig

* fix(Shared): bus creation via runtime

* refactor(Shared): NewWithAddress -> NewNodeWithAddress

* refactor(Shared): improved Create, making all modules unexported

* refactor(Consensus): consistency in naming paceMaker

* refactor(Shared): runtime -> runtimeCfg

* refactor(Shared): code review feedback

* fix(Shared): bugfixes

* fix(Consensus): merge fix

* test(Consensus): fixed tests (StoreBlock and InsertBlock mocks cfg)

* fix(Shared): removed config and genesis from bus

* style(Runtime): cleanup

* style(Runtime): import alias

* fix(Shared): module creation failures returns err

* refactor(Persistence): reverted converters to shared

* feat(Tooling): check_cross_module_imports makefile target

* style(Runtime): import names

* refactor(Shared): refactored clock to be sourced from runtimeMgr

* fix(Makefile): excluding makefile!

* feat(Tooling): improved check_cross_module_imports

* docs(Shared): Updated README and CHANGELOGs

* docs(Runtime): missing items

* test(Persistence): Tests: added missing mock configuration

Closes 262

* fix(Tooling): execute tests sequentially fix (wrong syntax)

* style(Makefile): check_cross_module_imports

* Update app/client/main.go

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

* refactor(Consensus): keys -> validatorKeys

* Update consensus/consensus_tests/utils_test.go

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

* Update consensus/consensus_tests/utils_test.go

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

* refactor(Consensu): NewNodeWithAddress -> NewNodeWithP2PAddress

* fix(Makefile): revert silent

* style(Shared): init var before return

* Update shared/test_artifacts/generator.go

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

* Update utility/test/module_test.go

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

* Update consensus/module.go

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

* Update consensus/types/converters.go

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

* refactor(Consensus): s/ActorListToMap/ActorListToValidatorMap

* Update consensus/types/converters.go

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

* fix(Consensus): converters fix

* refactor(Shared): preallocations

* docs(Shared): nits

* style(Persistence): cleanup

* Update shared/modules/module.go

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

* fix(Telemetry): error management

* Update shared/modules/module.go

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

* style(P2P): renamed p2pCfg

* fix(Tooling): reverted a line change introduced in #282

It was causing test failures because of inconsistent Postgresql state

* refactor(Shared): modules use interfaces for config and genesis

* refactor(Shared): using interfaces vs structs

* style(Shared): formatting

Goland is the offender... weird format on save rules... investigate

* style(Consensus): cleanup

* docs(Runtime): NewManagerFromReaders comment

* refactor(Shared): shared.test_artifacts -> runtime.test_artifacts

* fix(Persistence): leftover casts

* refactor(Shared): removed NodeModule

Signed-off-by: Alessandro De Blasis <[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
code health Nice to have code improvement consensus Consensus specific changes p2p P2P specific changes persistence Persistence specific changes utility Utility specific changes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants