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

chore(all): the big Sunday interfaces cleanup #2867

Merged
merged 7 commits into from
Dec 16, 2022
Merged

chore(all): the big Sunday interfaces cleanup #2867

merged 7 commits into from
Dec 16, 2022

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Oct 2, 2022 •
edited
Loading

Changes

⚠️ this unblocks a lot of things such as:

  • Mocking: drop mockery, local _test.go mocks only, merge mock generations, add a CI check for mocks without a go:generate command
  • Database: Dependency inject databases with narrow interfaces (based on this branch)
  • Runtime: change runtime code to only be wasmer (not 2 impl), then replace wasmer with wazero

🌞 Sunday fun / Canada Thanksgiving ™️ (also long overdued not-that-easy house cleaning)

  • Accept narrower interfaces (created in interfaces.go)
  • Do not import interfaces defined from other packages unless necessary (i.e. when an interface is returned)
  • Move/duplicate some mocks around (especially in dot/rpc) to make this possible
  • Remove unused interfaces
  • Remove unused methods in existing interfaces
  • Remove interface methods only used in tests (prod code shouldn't have to inject something just so test can work, tests should type assert the interface instead)

Tests

go test -tags integration github.com/ChainSafe/gossamer/...

Issues

#2088

Primary Reviewer

@timwu20

@codecov
Copy link

codecov bot commented Oct 2, 2022 •
edited
Loading

Codecov Report

Merging #2867 (1e9d87c) into development (4442eee) will increase coverage by 0.03%.
The diff coverage is 45.79%.

Additional details and impacted files 
@@               Coverage Diff               @@
##           development    #2867      +/-   ##
===============================================
+ Coverage        51.77%   51.81%   +0.03%     
===============================================
  Files              220      220              
  Lines            27764    27755       -9     
===============================================
+ Hits             14374    14380       +6     
+ Misses           12162    12149      -13     
+ Partials          1228     1226       -2

@qdm12 qdm12 changed the title chore(all): accept local narrow interfaces chore(all): the big Sunday interfaces cleanup Oct 2, 2022
@qdm12 qdm12 force-pushed the qdm12/local-interfaces branch 3 times, most recently from f7ff6f9 to 66c4cca Compare October 3, 2022 07:28
EclesioMeloJunior
EclesioMeloJunior requested changes Oct 3, 2022
View reviewed changes
Copy link
Member

@EclesioMeloJunior EclesioMeloJunior left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice PR so far 🎉 💪 , just small comments, didn't look at all those files

lib/keystore/helpers.go Show resolved Hide resolved
lib/keystore/helpers.go Outdated Show resolved Hide resolved
@qdm12 qdm12 force-pushed the qdm12/local-interfaces branch 2 times, most recently from 8b6365b to 6abb66d Compare October 11, 2022 07:05
@qdm12 qdm12 mentioned this pull request Oct 11, 2022
Merged
@qdm12 qdm12 marked this pull request as ready for review October 11, 2022 07:53
@qdm12 qdm12 force-pushed the qdm12/local-interfaces branch 2 times, most recently from ccb0f9b to d1f90f9 Compare October 11, 2022 11:02
@qdm12 qdm12 mentioned this pull request Oct 25, 2022
Merged
@qdm12 qdm12 force-pushed the qdm12/local-interfaces branch 2 times, most recently from e633108 to cdccb65 Compare November 2, 2022 17:50
@qdm12 qdm12 force-pushed the qdm12/local-interfaces branch 6 times, most recently from cc82734 to dc911f1 Compare November 9, 2022 13:31
@qdm12 qdm12 force-pushed the qdm12/local-interfaces branch 2 times, most recently from ddc2964 to 690e09c Compare November 24, 2022 15:04
@qdm12 qdm12 force-pushed the qdm12/local-interfaces branch 3 times, most recently from a969af9 to f89c898 Compare November 29, 2022 11:30
qdm12
qdm12 commented Nov 29, 2022
View reviewed changes
dot/state/grandpa.go Show resolved Hide resolved
dot/state/grandpa.go Show resolved Hide resolved
dot/state/epoch.go Show resolved Hide resolved
dot/state/epoch.go Show resolved Hide resolved
dot/state/block.go Show resolved Hide resolved
dot/state/block.go Show resolved Hide resolved
dot/state/pruner/pruner.go Show resolved Hide resolved
dot/state/service.go Show resolved Hide resolved
dot/state/storage.go Show resolved Hide resolved
@qdm12 qdm12 force-pushed the qdm12/local-interfaces branch 2 times, most recently from b6e37ee to 4982a91 Compare December 10, 2022 16:08
@qdm12 qdm12 mentioned this pull request Dec 12, 2022
Merged
jimjbrettj
jimjbrettj reviewed Dec 16, 2022
View reviewed changes
dot/node_test.go Outdated Show resolved Hide resolved
jimjbrettj
jimjbrettj approved these changes Dec 16, 2022
View reviewed changes
Copy link
Contributor

@jimjbrettj jimjbrettj left a comment

Choose a reason for hiding this comment

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

Really great work and a huge step toward cleaning up our tests/code. Awesome stuff

@timwu20
Copy link
Contributor

timwu20 commented Dec 16, 2022

nit: some of the interface files are named interface.go and interfaces.go. Should probably be consistent.

@qdm12 qdm12 merged commit db64e1b into development Dec 16, 2022
@qdm12 qdm12 deleted the qdm12/local-interfaces branch December 16, 2022 18:35
@github-actions GitHub Actions
Copy link

🎉 This PR is included in version 0.8.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimjbrettj jimjbrettj jimjbrettj approved these changes

@EclesioMeloJunior EclesioMeloJunior EclesioMeloJunior approved these changes

@timwu20 timwu20 timwu20 approved these changes

@noot noot Awaiting requested review from noot

@edwardmack edwardmack Awaiting requested review from edwardmack

@kishansagathiya kishansagathiya Awaiting requested review from kishansagathiya kishansagathiya is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@qdm12 @timwu20 @EclesioMeloJunior @jimjbrettj