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.

Sign up for GitHub

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

Jump to bottom

[Bug] Fix broken consensus tests on main branch - mocking StoreBlock in persistence module #259

Merged
merged 8 commits into from
Oct 4, 2022
Merged

[Bug] Fix broken consensus tests on main branch - mocking StoreBlock in persistence module #259

merged 8 commits into from
Oct 4, 2022

Conversation

Olshansk
Copy link
Member

@Olshansk Olshansk commented Sep 29, 2022 •
edited
Loading

Description

Fix a small bug on main introduced by removal of UtilityModule's StoreBlock function.

Issue

No associated issue. This is a quick followup to #254.

Screen Shot 2022-09-29 at 4 32 23 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

  • Fixed broken test on main by mocking the StoreBlock function on the persistence module in a consensus test
  • Remove the unused StoreBlock function in the utility module

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 the bug Something isn't working - expected behaviour is incorrect label Sep 29, 2022
@Olshansk Olshansk self-assigned this Sep 29, 2022
@Olshansk Olshansk changed the title Fix broken test on main [Bug] Fix broken consensus tests on main branch - mocking StoreBlock in persistence module Sep 29, 2022
@andrewnguyen22
Copy link
Contributor

Finish PR description before requesting review

@Olshansk Olshansk marked this pull request as ready for review September 29, 2022 23:36
@Olshansk Olshansk requested a review from a team as a code owner September 29, 2022 23:36
andrewnguyen22
andrewnguyen22 suggested changes Sep 29, 2022
View reviewed changes
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.

Versioning OB1

utility/doc/CHANGELOG.md Outdated Show resolved Hide resolved
@Olshansk
Copy link
Member Author

@andrewnguyen22

Finish PR description before requesting review

Sounds good. I jumped the 🔫 on this one.

@Olshansk
Copy link
Member Author

Versioning...

giphy

@Olshansk
Copy link
Member Author

Please note that make develop_test works but there is still an issue with LocalNet we're going to need to fix.

Screen Shot 2022-09-29 at 5 51 19 PM

@Olshansk Olshansk requested a review from deblasis October 3, 2022 22:00
andrewnguyen22
andrewnguyen22 approved these changes Oct 4, 2022
View reviewed changes
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.

@Olshansk LGTM, I realized you asked me to fix actor_test before merging and I did not see that until after I had merged. I planned on following up with another PR, but was wondering if you'd be able to patch that up in this PR.

If not NBD -lmk

@Olshansk Olshansk merged commit 92582b5 into main Oct 4, 2022
@Olshansk Olshansk deleted the main_broke_test/StoreBlock branch October 4, 2022 00:23
@Olshansk
Copy link
Member Author

Olshansk commented Oct 4, 2022

@andrewnguyen22 Patched, updated, pushed, verified & merged.

@Olshansk
Copy link
Member Author

Olshansk commented Oct 4, 2022

Also, I just misunderstood what you meant by I realized you asked me to fix actor_test before merging. Oddly enough make develop_test passed for me, so:

  1. Can you submit a PR with those unit test fixes.
  2. I will investigate why make develop_test passes

@andrewnguyen22
Copy link
Contributor

@Olshansk can't reproduce the issue - seems fine

@Olshansk Olshansk mentioned this pull request Oct 4, 2022
Merged
16 tasks
@Olshansk
Copy link
Member Author

Olshansk commented Oct 4, 2022

@andrewnguyen22 Following up here for posterity, fix is in #282

Olshansk added a commit that referenced this pull request Oct 7, 2022
@Olshansk
[TECHDEBT][UTILITY] Fix Unit Tests using testing.M (#282)
983f480 
## 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 <!-- add details here if it a different type of change -->

## 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewnguyen22 andrewnguyen22 andrewnguyen22 approved these changes

@deblasis deblasis Awaiting requested review from deblasis

Assignees

@Olshansk Olshansk

Labels
bug Something isn't working - expected behaviour is incorrect
Projects
V1 Dashboard
Status: Done
Milestone
M1: Pocket PoS (Proof of Stake)
Development

Successfully merging this pull request may close these issues.
2 participants
@Olshansk @andrewnguyen22