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 tests #1130

Closed
wants to merge 10 commits into from
Closed

lint tests #1130

wants to merge 10 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Dec 17, 2022

The cosmos-sdk, ibc-go and gaia all fail to lint tests, leading to
inconsistencies. This PR addresses that.

@faddat faddat marked this pull request as ready for review December 18, 2022 06:59
@faddat faddat requested a review from alpe as a code owner December 18, 2022 06:59
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
No Duplication information No Duplication information

@alpe
Copy link
Contributor

alpe commented Dec 20, 2022

Thank you for the PR! This was probably a lot of work. I would like to get some more feedback if we really want to enforce linter constraints on tests. This was sometimes a heated debate on previous projects.

@faddat
Copy link
Contributor Author

faddat commented Dec 25, 2022

Opinion: it's important, and results in better tests.

Best example for testing is errcheck.

While we test, we often don't check errors, and in the long run this could lead to faulty tests.

@alpe
Copy link
Contributor

alpe commented Jan 9, 2023

@Mergifyio rebase

There was no other feedback. So my only concern now is that this is going to create tons of merge conflicts with develop_sdk47 after #1136

@mergify
Copy link
Contributor

mergify bot commented Jan 9, 2023

rebase

❌ Base branch update has failed

faddat token is invalid, make sure faddat can still log in on the Mergify dashboard.
err-code: CC62D

@alpe
Copy link
Contributor

alpe commented Jan 19, 2023

There are already a lot of merge conflicts with #1149 . I appreciate your effort and energy but in order to reduce the workload while maintaining 2 active development branches, I close this now.
Please note, there was no resistance to have tests coded covered, so we can try again after #1149 is merged to main.

@alpe alpe closed this Jan 19, 2023
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.

2 participants