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

## Description Fix unit tests that exist on the main branch. ## Origin Document Fixes conversation in #259: <img width="1059" alt="Screen Shot 2022-10-04 at 4 07 23 PM" src="https://user-images.githubusercontent.com/1892194/193946742-8a9f3914-86f1-4ccb-92fb-a1b025c5258a.png"> ## Issue Rather than the [core Go documentation](https://pkg.go.dev/testing), I believe [this article](https://medium.com/goingogo/why-use-testmain-for-testing-in-go-dafb52b406bc) does a better job at explaining why we had broken tests: <img width="780" alt="Screen Shot 2022-10-04 at 4 12 38 PM" src="https://user-images.githubusercontent.com/1892194/193947366-5cd62ef3-a01a-4d74-95d3-45929ee9348a.png"> Before this change, even when a unit test failed, we still got a pass: <img width="1241" alt="Screen Shot 2022-10-04 at 4 26 09 PM" src="https://user-images.githubusercontent.com/1892194/193948965-9079896e-5ab0-40dc-8f1b-f8bdcf4593c2.png"> ## Type of change Please mark the relevant option(s): - [ ] New feature, functionality or library - [x] Bug fix - [x] 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 - [x] `make develop_test` - [x] [LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md) w/ all of the steps outlined in the `README` ## Required Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have tested my changes using the available tooling - [x] I have updated the corresponding CHANGELOG ### If Applicable Checklist - [x] I have updated the corresponding README(s); local and/or global - [x] I have added tests that prove my fix is effective or that my feature works - [ ] I have added, or updated, [mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding README(s) - [ ] I have added, or updated, documentation and [mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*` if I updated `shared/*`README(s)
pokt-network · Oct 7, 2022 · 983f480 · 983f480
1 parent 23055a0
commit 983f480
Show file tree

Hide file tree

Showing 17 changed files with 111 additions and 127 deletions.
diff --git a/Makefile b/Makefile
@@ -263,15 +263,10 @@ test_all_with_coverage: # generate_mocks
 test_race: # generate_mocks
 	go test ${VERBOSE_TEST} -race ./...
 
-.PHONY: test_utility_module
+.PHONY: test_utility
 ## Run all go utility module unit tests
-test_utility_module: # generate_mocks
-	go test ${VERBOSE_TEST} -p 1 -count=1  ./shared/tests/utility_module/...
-
-.PHONY: test_utility_types
-## Run all go utility types module unit tests
-test_utility_types: # generate_mocks
-	go test ${VERBOSE_TEST} ./utility/types/...
+test_utility: # generate_mocks
+	go test ${VERBOSE_TEST} -p=1 -count=1  ./utility/...
 
 .PHONY: test_shared
 ## Run all go unit tests in the shared module

diff --git a/consensus/CHANGELOG.md b/consensus/CHANGELOG.md
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.0.0.5] - 2022-10-06
+
+- Don't ignore the exit code of `m.Run()` in the unit tests
+
 ## [0.0.0.4] - 2022-09-28
 
 - `consensusModule` stores block directly to prevent shared structure in the `utilityModule`

diff --git a/consensus/consensus_tests/utils_test.go b/consensus/consensus_tests/utils_test.go
@@ -31,9 +31,10 @@ import (
 )
 
 func TestMain(m *testing.M) {
-	m.Run()
+	exitCode := m.Run()
 	os.Remove(testingConfigFilePath)
 	os.Remove(testingGenesisFilePath)
+	os.Exit(exitCode)
 }
 
 // If this is set to true, consensus unit tests will fail if additional unexpected messages are received.

diff --git a/p2p/CHANGELOG.md b/p2p/CHANGELOG.md
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.0.0.4] - 2022-10-06
+
+- Don't ignore the exit code of `m.Run()` in the unit tests
+
 ## [0.0.0.3] - 2022-09-15
 
 **[TECHDEBT] AddrBook management optimization and refactoring** [#246](github.com/pokt-network/pocket/issues/246)

diff --git a/p2p/module_raintree_test.go b/p2p/module_raintree_test.go
@@ -449,9 +449,13 @@ func createGenesisState(t *testing.T, valKeys []cryptoPocket.PrivateKey) modules
 }
 
 func TestMain(m *testing.M) {
-	m.Run()
-	files, _ := filepath.Glob("*.json")
+	exitCode := m.Run()
+	files, err := filepath.Glob("*.json")
+	if err != nil {
+		log.Fatalf("Error finding json file: %v", err)
+	}
 	for _, f := range files {
 		os.Remove(f)
 	}
+	os.Exit(exitCode)
 }
diff --git a/persistence/CHANGELOG.md b/persistence/CHANGELOG.md
@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.0.0.6] - 2022-10-06
+
+- Don't ignore the exit code of `m.Run()` in the unit tests
+- Fixed several broken unit tests related to type casting
+
 ## [0.0.0.5] - 2022-09-14
 
 - Consolidated `PostgresContext` and `PostgresDb` into a single structure

diff --git a/persistence/shared_sql.go b/persistence/shared_sql.go
@@ -14,10 +14,10 @@ import (
 // for the purpose of being explicit: https://github.com/pokt-network/pocket/pull/140#discussion_r939731342
 // TODO Cleanup with #149
 const (
-	UndefinedStakingStatus = 0
-	UnstakingStatus        = 1
-	StakedStatus           = 2
-	UnstakedStatus         = 3
+	UndefinedStakingStatus = int32(0)
+	UnstakingStatus        = int32(1)
+	StakedStatus           = int32(2)
+	UnstakedStatus         = int32(3)
 )
 
 func UnstakingHeightToStatus(unstakingHeight int64) int32 {

diff --git a/persistence/test/account_test.go b/persistence/test/account_test.go
@@ -3,13 +3,14 @@ package test
 import (
 	"encoding/hex"
 	"fmt"
-	"github.com/pokt-network/pocket/persistence/types"
-	"github.com/pokt-network/pocket/shared/modules"
 	"log"
 	"math/big"
 	"math/rand"
 	"testing"
 
+	"github.com/pokt-network/pocket/persistence/types"
+	"github.com/pokt-network/pocket/shared/modules"
+
 	"github.com/pokt-network/pocket/persistence"
 	"github.com/pokt-network/pocket/shared/crypto"
 	"github.com/stretchr/testify/require"
@@ -312,7 +313,7 @@ func TestGetAllPools(t *testing.T) {
 		return db.AddPoolAmount(pool.GetAddress(), "10")
 	}
 
-	getAllActorsTest(t, db, db.GetAllPools, createAndInsertNewPool, updatePool, 6)
+	getAllActorsTest(t, db, db.GetAllPools, createAndInsertNewPool, updatePool, 7)
 }
 
 // --- Helpers ---

diff --git a/persistence/test/application_test.go b/persistence/test/application_test.go
@@ -135,12 +135,12 @@ func TestGetAppStatus(t *testing.T) {
 	// Check status before the app exists
 	status, err := db.GetAppStatus(addrBz, 0)
 	require.Error(t, err)
-	require.Equal(t, status, persistence.UndefinedStakingStatus, "unexpected status")
+	require.Equal(t, persistence.UndefinedStakingStatus, status, "unexpected status")
 
 	// Check status after the app exists
 	status, err = db.GetAppStatus(addrBz, 1)
 	require.NoError(t, err)
-	require.Equal(t, status, DefaultStakeStatus, "unexpected status")
+	require.Equal(t, DefaultStakeStatus, status, "unexpected status")
 }
 
 func TestGetAppPauseHeightIfExists(t *testing.T) {

diff --git a/persistence/test/service_node_test.go b/persistence/test/service_node_test.go
@@ -143,12 +143,12 @@ func TestGetServiceNodeStatus(t *testing.T) {
 	// Check status before the serviceNode exists
 	status, err := db.GetServiceNodeStatus(addrBz, 0)
 	require.Error(t, err)
-	require.Equal(t, status, persistence.UndefinedStakingStatus, "unexpected status")
+	require.Equal(t, persistence.UndefinedStakingStatus, status, "unexpected status")
 
 	// Check status after the serviceNode exists
 	status, err = db.GetServiceNodeStatus(addrBz, 1)
 	require.NoError(t, err)
-	require.Equal(t, status, DefaultStakeStatus, "unexpected status")
+	require.Equal(t, DefaultStakeStatus, status, "unexpected status")
 }
 
 func TestGetServiceNodePauseHeightIfExists(t *testing.T) {

diff --git a/persistence/test/setup_test.go b/persistence/test/setup_test.go
@@ -55,10 +55,11 @@ var testPersistenceMod modules.PersistenceModule // initialized in TestMain
 func TestMain(m *testing.M) {
 	pool, resource, dbUrl := sharedTest.SetupPostgresDocker()
 	testPersistenceMod = newTestPersistenceModule(dbUrl)
-	m.Run()
+	exitCode := m.Run()
 	os.Remove(testingConfigFilePath)
 	os.Remove(testingGenesisFilePath)
 	sharedTest.CleanupPostgresDocker(m, pool, resource)
+	os.Exit(exitCode)
 }
 
 func NewTestPostgresContext(t *testing.T, height int64) *persistence.PostgresContext {

diff --git a/persistence/test/validator_test.go b/persistence/test/validator_test.go
@@ -140,12 +140,12 @@ func TestGetValidatorStatus(t *testing.T) {
 	// Check status before the validator exists
 	status, err := db.GetValidatorStatus(addrBz, 0)
 	require.Error(t, err)
-	require.Equal(t, status, persistence.UndefinedStakingStatus, "unexpected status")
+	require.Equal(t, persistence.UndefinedStakingStatus, status, "unexpected status")
 
 	// Check status after the validator exists
 	status, err = db.GetValidatorStatus(addrBz, 1)
 	require.NoError(t, err)
-	require.Equal(t, status, DefaultStakeStatus, "unexpected status")
+	require.Equal(t, DefaultStakeStatus, status, "unexpected status")
 }
 
 func TestGetValidatorPauseHeightIfExists(t *testing.T) {

diff --git a/shared/test_artifacts/util.go b/shared/test_artifacts/util.go
@@ -92,7 +92,6 @@ func CleanupPostgresDocker(_ *testing.M, pool *dockertest.Pool, resource *docker
 	if err := pool.Purge(resource); err != nil {
 		log.Fatalf("could not purge resource: %s", err)
 	}
-	os.Exit(0)
 }
 
 // TODO(drewsky): Remove this in favor of a golang specific solution

diff --git a/utility/doc/CHANGELOG.md b/utility/doc/CHANGELOG.md
@@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.0.0.6] - 2022-10-06
+
+- Don't ignore the exit code of `m.Run()` in the unit tests
+- Fixed several broken unit tests related to type casting
+- Removed some unit tests (e.g. `TestUtilityContext_UnstakesPausedBefore`) that were legacy and replaced by more general ones (e.g. `TestUtilityContext_UnstakePausedBefore`)
+- Avoid exporting unnecessary test helpers
+
 ## [0.0.0.5] - 2022-09-29
 
 - Remove unused `StoreBlock` function from the utility module interface
@@ -50,7 +57,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `UtilityConfig` uses shared interfaces in order to accept `MockUtilityConfig` in test_artifacts
 - Moved all utilty tests from shared to tests package
 - Left `TODO` for tests package still importing persistence for `NewTestPersistenceModule`
-    - This is one of the last places where cross-module importing exists
+  - This is one of the last places where cross-module importing exists
 
 ## [0.0.1] - 2022-07-20
 
@@ -70,18 +77,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added
 
 - Added minimal 'proof of stake' implementation with few Pocket Specific terminologies and actors
-    - Structures
-        - Accounts
-        - Validators
-        - Fishermen
-        - Applications
-        - Service Nodes
-        - Pools
-    - Messages
-        - Stake
-        - Unstake
-        - EditStake
-        - Pause
-        - Unpause
-        - Send
+  - Structures
+    - Accounts
+    - Validators
+    - Fishermen
+    - Applications
+    - Service Nodes
+    - Pools
+  - Messages
+    - Stake
+    - Unstake
+    - EditStake
+    - Pause
+    - Unpause
+    - Send
 - Added initial governance params