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

Fix bug: false positive warning for typed function calls #92

Merged
merged 1 commit into from
Jun 27, 2023
Merged

Conversation

nunnatsa
Copy link
Owner

Description

When calling a function that return a type in the async assertion (eventually/constantly) body, if this type is a function, the linter fails to recognize it and fire a warning.

This PR fixes this bug, by checking the underline types.

Fixes #91

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Added test case and related test data
  • Update the functional test

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I ran golangci-lint

@nunnatsa

@nunnatsa
Copy link
Owner Author

@pohly, would you mind to take a look at this fix?

@github-actions
Copy link

github-actions bot commented Jun 27, 2023

Pull Request Test Coverage Report for Build 5391259784

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 84.669%

Totals Coverage Status
Change from base Build 5391215987: 0.08%
Covered Lines: 845
Relevant Lines: 998

💛 - Coveralls

Copy link

@pohly pohly left a comment

Choose a reason for hiding this comment

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

I can confirm that this fixes the issue in Kubernetes. Thanks!

Just some minor nits.

ginkgo_linter.go Outdated
@@ -371,6 +368,19 @@ func checkAsyncAssertion(pass *analysis.Pass, config types.Config, expr *ast.Cal
return true
}

func isValidFuncCallInAsync(t gotypes.Type) bool {
Copy link

@pohly pohly Jun 27, 2023

Choose a reason for hiding this comment

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

I was initially confused about the FuncCall here. This isn't checking the function call, it's checking the type of the return value of the function call, is it?

Perhaps rename to isValidAsyncValueType?

func isValidFuncCallInAsync(t gotypes.Type) bool {
switch t.(type) {
// allow functions that return function or channel.
case *gotypes.Signature, *gotypes.Chan, *gotypes.Pointer:
Copy link

Choose a reason for hiding this comment

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

Nit: I find the intention easier to read when the return true is here instead of below.

When calling a function that return a type in the async
assertion (eventually/constantly) body, if this type is a function, the
linter fiels to recognize it and fire a warning.

This PR fixes this bug, by checking the underline types.
@nunnatsa nunnatsa merged commit 31aabfa into main Jun 27, 2023
@nunnatsa nunnatsa deleted the fix-91 branch June 27, 2023 15:03
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.

[BUG] false positive for Eventually/Consistently
2 participants