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

🌱 disable more style linters for test files #3707

Merged
merged 5 commits into from
Dec 4, 2023

Conversation

spencerschrock
Copy link
Member

What kind of change does this PR introduce?

linter update

What is the current behavior?

Test writers need to either satisfy the linter, workaround the linter, or add a nolint directive. For something that's more or less a style decision in tests, this is bad. See #3688 (comment)

What is the new behavior (if this is a feature change)?**

These linters are disabled, as really they're stylistic, and not important in tests. This is to some extent a followup of #2844, which disabled some already.

  • lll
  • goerr113
  • wrapcheck

I also fixed a few unrelated linter warnings because it was easier to just fix the problem than add an exception because they weren't widespread:

  • a few instances of errcheck
  • one test which gosec complained about file permissions
  • an un-needed type conversion for unconvert
  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

NONE

Special notes for your reviewer

When evaluating what linters to disable, I just counted what nolint directives are used in test files and targeted the big ones.

Note: we can't disable govet, because some of what it catches is important (see #2844 (comment))

grep -oinr --include \*_test.go -E "//nolint:[^ ]*" | cut -d ":" -f 4 | sort | uniq -c | sort -n
      1 lll,gocritic
      1 lll,govet
      1 nestif
      1 tparallel,paralleltest
      1 unconvert
      1 wrapcheck,lll
      2 gosec
      2 govet,goerr113
      3 govet,lll
      4 staticcheck
      5 wrapcheck
      8 gocognit
      9 errcheck
     23 paralleltest
     33 goerr113
     35 stylecheck
     44 lll
    142 govet

Does this PR introduce a user-facing change?

For user-facing changes, please add a concise, human-readable release note to
the release-note

(In particular, describe what changes users might need to make in their
application as a result of this pull request.)


@spencerschrock spencerschrock requested a review from a team as a code owner November 30, 2023 00:09
@spencerschrock spencerschrock requested review from justaugustus and raghavkaul and removed request for a team November 30, 2023 00:09
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Merging #3707 (4ee7114) into main (4d1621b) will decrease coverage by 7.67%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3707      +/-   ##
==========================================
- Coverage   76.35%   68.69%   -7.67%     
==========================================
  Files         210      210              
  Lines       14395    14395              
==========================================
- Hits        10991     9888    -1103     
- Misses       2757     3934    +1177     
+ Partials      647      573      -74     

@justaugustus justaugustus enabled auto-merge (squash) December 4, 2023 02:05
Copy link
Member

@justaugustus justaugustus left a comment

Choose a reason for hiding this comment

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

Thanks @spencerschrock!

@justaugustus justaugustus merged commit 1625b0c into ossf:main Dec 4, 2023
38 checks passed
@spencerschrock spencerschrock deleted the lint/no-lll-tests branch December 4, 2023 16:44
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