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

chore: enable errcheck linter #4162

Merged
merged 18 commits into from
Jun 18, 2024
Merged

chore: enable errcheck linter #4162

merged 18 commits into from
Jun 18, 2024

Conversation

faddat
Copy link
Contributor

@faddat faddat commented May 19, 2024

  • adds errcheck linter

Description

This PR fixes a bug in how flag tests are handled. Since we're now checking if the test runs return error, we are able to see that flag tests have been failing because they don't include a value.

Link to earlier issue

I think that this PR shows the same behavior showed in #4074. Specifically, It set off the race detector. I don't think that is because of the linting changes. The difference between:

and

is intentionally just a single comment.

Running tests on ubuntu fails in the first commit, but passes in the second. So, what I'm suggesting here is that we should make a PR that runs the tests 10x against the current main branch. This will let us observe the tests and know if they are flaky.

But it doesn't seem to be related to the errcheck work. Here's the data race here:

That's from main. I think we should spend some additional time looking for data races.


Please make sure to check the following for your PR:

  • This PR complies with the contributing guidelines.
  • Reviewed "Files changed" and left comments if necessary
  • Included relevant documentation changes.

Ignite CLI team only:

  • I have updated the Unreleased section in the changelog.md for my changes.
  • If the templates in ignite/templates/files have been changed, make
    sure that the change doesn't need to be reflected in the
    ignite/templates/files-* folders.

@faddat
Copy link
Contributor Author

faddat commented May 19, 2024

this will show the bug surfaced in #4156

@faddat faddat marked this pull request as draft May 19, 2024 21:28
@faddat faddat mentioned this pull request May 19, 2024
4 tasks
@faddat faddat marked this pull request as ready for review May 20, 2024 08:01
@faddat faddat mentioned this pull request May 21, 2024
Copy link
Collaborator

@Pantani Pantani left a comment

Choose a reason for hiding this comment

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

LGMT!!! thanks for the contribution @faddat

Just one comment

ignite/services/chain/init.go Show resolved Hide resolved
@faddat
Copy link
Contributor Author

faddat commented May 23, 2024

I'll resolve conflicts later today

@Pantani
Copy link
Collaborator

Pantani commented May 27, 2024

Error: ../../pkg/cosmosgen/cosmosgen.go:133:15: b.Cleanup undefined (type cosmosbuf.Buf has no field or method Cleanup)

the cleanup method from the cosmosbuf pkg was removed

Pantani
Pantani previously approved these changes Jun 13, 2024
# Conflicts:
#	ignite/internal/plugin/testdata/execute_fail/go.mod
#	ignite/internal/plugin/testdata/execute_ok/go.mod
@julienrbrt julienrbrt enabled auto-merge (squash) June 13, 2024 18:11
@julienrbrt julienrbrt disabled auto-merge June 18, 2024 15:02
@julienrbrt julienrbrt enabled auto-merge (squash) June 18, 2024 15:03
@julienrbrt julienrbrt merged commit 52be059 into ignite:main Jun 18, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants