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

lint all tests: mark 2 #6083

Closed
wants to merge 158 commits into from
Closed

lint all tests: mark 2 #6083

wants to merge 158 commits into from

Conversation

faddat
Copy link
Member

@faddat faddat commented Aug 16, 2023 •
edited
Loading

What is the purpose of the change

Replace #5127 by merging from the latest working commit in #5127 to sidestep a
bug.

Testing and Verifying

(Please pick one of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added unit test that validates ...
  • Added integration tests for end-to-end deployment with ...
  • Extended integration test for ...
  • Manually verified the change by ...

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

DongLieu and others added 30 commits April 27, 2023 15:34
@DongLieu
TestJoinSwapExactAmountInConsistency
70cfbdc
@DongLieu
add cases corners
37074cc
@faddat
run golangci-lint and resolve ineffectual assignment to err
f5dc71d
@faddat
go work sync
9fa7369
@faddat
waypoint in "check all errors in tests"
c688011
@faddat
50 error checks remaining
ac5bcd2
@faddat
42 errors remaining to check in tests
3105a8a
@faddat
all tests pass and as many errors as possible have been checked
cbed0c4
@faddat
revert enabling golangci-lint to check tests, as it would always fail…
31327a6 
… afterwards
@faddat
update changelog
2acfd0e
@faddat
use thelper in golangci-lint without linting every test
1e9eb0c
@faddat @p0mvn
Update tests/e2e/containers/containers.go
d624769 
Co-authored-by: Roman <[email protected]>
@faddat
Merge remote-tracking branch 'origin/main' into faddat/check-all-errors
e9c621f
@faddat
thelper for incentives bench test
d616f90
@faddat
unused code in tests
0a59632
@faddat @czarcas7ic
Update x/concentrated-liquidity/position_test.go
8ae270d 
Co-authored-by: Adam Tucker <[email protected]>
@faddat
Merge branch 'main' into faddat/check-all-errors
ddc5487
@faddat
fix forced type assertions
560388c
@faddat
revert change to linter config
b70f80a
@faddat @pysel
Update tests/e2e/e2e_test.go
b2d1fab 
Co-authored-by: Ruslan Akhtariev <[email protected]>
@faddat @pysel
Update x/concentrated-liquidity/pool_test.go
85c7ecb 
Co-authored-by: Ruslan Akhtariev <[email protected]>
@faddat
skip pool = poolI
8e573d7
@faddat
update swap_test.go to test that cl type assertions fail
eb9cc68
@faddat
trial
56c6152
@faddat
we needed pool because it's used to CalcInGivenOut
daa610e
@faddat
Merge branch 'main' into faddat/check-all-errors
93f5581
@faddat
Merge remote-tracking branch 'origin/main' into faddat/check-all-errors
e243555
@faddat
re-add skip state sync
16c6c97
@faddat
Merge branch 'faddat/unused-in-tests' into accumulate
39f6fc4
@faddat
Merge branch 'faddat/forcetypeassert' into accumulate
6bc25aa
@faddat faddat added A:no-changelog A:backport/v17.x backport patches to v17.x branch V:state/compatible/backport State machine compatible PR, should be backported labels Aug 16, 2023
@faddat faddat mentioned this pull request Aug 16, 2023
Closed
@p0mvn
Copy link
Member

p0mvn commented Aug 18, 2023

Hi @faddat . Thanks for this effort.

Once ready, is it possible to merge this incrementally in separate PRs? For example, module by module or any other split that would make the review and merge process more streamlined?

@faddat
Copy link
Member Author

faddat commented Aug 24, 2023 •
edited
Loading

Yes, it is.

Please let me propose a path to merging this:

  1. I'll merge main and lint whatever else needs linting
  2. I'll break this into per-module PR's
  3. those'll get reviewed and merged individually
  4. at the end I probably need to do a single PR to cleanup anything that diverged from the standard in the meantime.

Thanks for the suggestion!

@faddat faddat force-pushed the faddat/lint-all-tests branch from b207589 to 3083bd6 Compare August 24, 2023 03:29
@faddat faddat force-pushed the faddat/lint-all-tests branch from 67e0ef5 to 2780341 Compare August 25, 2023 15:21
@github-actions
Copy link
Contributor

Important Notice

This PR includes modifications to the tests/e2e/initialization module.
Please follow the instructions below:

  1. Backport these changes to the previous Osmosis version's branch.
  2. Run the script inside a Docker container to update genesis and configs for pre-upgrade Osmosis.
  3. Merge the backported changes.
  4. The image will be built and uploaded to Docker Hub here.
  5. Grab the latest image and update it in the PR to the main branch replacing the previousVersionInitTag in the osmosis/tests/e2e/containers/config.go

Please let us know if you need any help.

@github-actions github-actions bot added C:x/mint C:x/txfees C:simulator Edits simulator or simulations labels Aug 25, 2023
@faddat
Copy link
Member Author

faddat commented Aug 25, 2023

I'll break this up tomorrow.

@faddat faddat closed this Sep 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattverse mattverse Awaiting requested review from mattverse mattverse will be requested when the pull request is marked ready for review mattverse is a code owner

@nicolaslara nicolaslara Awaiting requested review from nicolaslara nicolaslara will be requested when the pull request is marked ready for review nicolaslara is a code owner

@p0mvn p0mvn Awaiting requested review from p0mvn p0mvn will be requested when the pull request is marked ready for review p0mvn is a code owner

@AlpinYukseloglu AlpinYukseloglu Awaiting requested review from AlpinYukseloglu AlpinYukseloglu will be requested when the pull request is marked ready for review AlpinYukseloglu is a code owner

@czarcas7ic czarcas7ic Awaiting requested review from czarcas7ic czarcas7ic will be requested when the pull request is marked ready for review czarcas7ic is a code owner

Assignees
No one assigned
Labels
A:backport/v17.x backport patches to v17.x branch A:no-changelog C:app-wiring Changes to the app folder C:CLI C:simulator Edits simulator or simulations C:x/concentrated-liquidity C:x/epochs C:x/gamm Changes, features and bugs related to the gamm module. C:x/incentives C:x/lockup C:x/mint C:x/pool-incentives C:x/poolmanager C:x/superfluid C:x/tokenfactory C:x/twap Changes to the twap module C:x/txfees T:build T:CI V:state/compatible/backport State machine compatible PR, should be backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@faddat @p0mvn @DongLieu