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

[Bug] Broken Tests on main after #235 #308

Closed
2 tasks
Olshansk opened this issue Oct 18, 2022 · 0 comments · Fixed by #309
Closed
2 tasks

[Bug] Broken Tests on main after #235 #308

Olshansk opened this issue Oct 18, 2022 · 0 comments · Fixed by #309
Assignees
Labels
bug Something isn't working - expected behaviour is incorrect

Comments

@Olshansk
Copy link
Member

Objective

Fix broken bugs on main introduced by #235.

Origin Document

#235 was a large PR that reintroduced a bug fixed by #282.

Specifically, in runtime/test_artifacts/uti.go, the addition of os.Exit(0)

func CleanupPostgresDocker(_ *testing.M, pool *dockertest.Pool, resource *dockertest.Resource) {
	// You can't defer this because `os.Exit`` doesn't care for defer
	if err := pool.Purge(resource); err != nil {
		log.Fatalf("could not purge resource: %s", err)
	}
	os.Exit(0)
}

Ignores the exitCode in persistence/test/setup_test.go

func TestMain(m *testing.M) {
	pool, resource, dbUrl := test_artifacts.SetupPostgresDocker()
	testPersistenceMod = newTestPersistenceModule(dbUrl)
	exitCode := m.Run()
	test_artifacts.CleanupPostgresDocker(m, pool, resource)
	os.Exit(exitCode)
}

Resulting in various errors committed to main:

Screen Shot 2022-10-18 at 10 12 52 AM

Doing a hard reset to the prior commit (git reset --hard 2c390c3c055b4d1a36fb93075867bcf96b89fdcd) validated that these tests were functional in the previous commit.

Goals

  • make develop_test should pass after removing os.Exit(0) from runtime/test_artifacts/util.go

Deliverable

  • Removal of os.Exit(0) in runtime/test_artifacts/util.go
  • Fixes in all broken tests raised by make test_all

Non-goals / Non-deliverables

  • New functionality
  • Refactoring / organizing existing code

Testing Methodology

  • All tests: make develop_test
  • LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md

Creator: @Olshansk
Co-Owners: @deblasis

@Olshansk Olshansk added the bug Something isn't working - expected behaviour is incorrect label Oct 18, 2022
@jessicadaugherty jessicadaugherty moved this to Up Next in V1 Dashboard Oct 18, 2022
@deblasis deblasis moved this from Up Next to In Progress in V1 Dashboard Oct 18, 2022
deblasis added a commit that referenced this issue Oct 24, 2022
## Description

This PR fixes a regression that was hiding an actual bug in the code.

## Issue

Fixes #308 

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [x] Bug fix
- [ ] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Making sure that the test harness returns the right return code to the OS
- Added missing mappings

## 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`
<!--
If you added additional tests or infrastructure, describe it here.

Bonus points for images and videos or gifs.
-->

## 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
- [x] I have tested my changes using the available tooling
- [x] 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](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)

* test(test_artifacts): removed os.Exit(0) that was bypassing some tests

* fix(Persistence): added missing properties to converter

* test(Utility): added missing mock config as per comment

#309 (comment)

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
Repository owner moved this from In Progress to Done in V1 Dashboard Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working - expected behaviour is incorrect
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants