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

lint interchaintest in the same manner as ibc-go #675

Closed
wants to merge 20 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Jul 24, 2023

This PR lints interchaintest in the same manner as ibc-go for consistency.

Where exclusions have been made, they've been noted in .golangci.yml.

This is somewhat pursuant to #713, but I think that'll require a second separate PR.

The goal is to add enforced consistency to the ibc-go codebase.

@faddat faddat requested a review from a team as a code owner July 24, 2023 14:15
@faddat faddat requested a review from DavidNix July 24, 2023 14:15
@faddat faddat closed this Jul 25, 2023
@faddat faddat reopened this Jul 25, 2023
@faddat faddat marked this pull request as draft July 25, 2023 16:04
@faddat faddat marked this pull request as ready for review August 21, 2023 09:38
@faddat faddat changed the title linting autofixes and unnecessary conversions lint interchaintest in the same manner as ibc-go Aug 23, 2023
@faddat
Copy link
Contributor Author

faddat commented Aug 24, 2023

@boojamya -- this PR and #696 have showed some odd behavior, is it possible that there is a "flaky test" situation or such?

Basically with #696 I was able to observe some failures, that I think shouldn't have been failures. Happy to help troubleshoot.

@boojamya
Copy link
Contributor

boojamya commented Aug 24, 2023

Yes, there are some flaky tests here. Especially that SQL unit test : (

Re-running the test now.

Also, can you add context here? This is this automated right?
If so, we may want to add a Makerfile command incase external contributes get hung up on linting errors.

We maintain n and n-1 versions of ibc-go, so we'll want to mimic this on our v6 branch.

@faddat
Copy link
Contributor Author

faddat commented Aug 29, 2023

Ah, so, the first commit contains the automated changes.

Otherwise, I recommend running:

golangci-lint run ./... to see the isuses

I can add this to the makefile :)

@faddat
Copy link
Contributor Author

faddat commented Sep 1, 2023

We maintain n and n-1 versions of ibc-go, so we'll want to mimic this on our v6 branch.

This PR is in preparation for v50, so is it cool if I focus on main (supporting v50) and v7 (supporting sdk47)?

If needed, I can lint v6 tho, just lmk

@faddat
Copy link
Contributor Author

faddat commented Sep 5, 2023

Is this flaky tests, or do you think there's an issue in the PR?

Thanks

@Reecepbcups
Copy link
Member

@faddat just flakey

@Reecepbcups
Copy link
Member

Going to close this as stale due to the # of conflicts & time the PR has been open.

The team has discussed re-opening in the future, but likely should be done from scratch once we agree on which options to use

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants