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

🐛 Ignore collection failures in non-tests #15

Merged
merged 4 commits into from
Nov 13, 2023

Conversation

stdedos
Copy link
Collaborator

@stdedos stdedos commented Oct 18, 2023

This change applies the pre-existing patterns to identify if the files with collection problems are tests. It is then used to eliminate the false-positives of F6401 (cannot-enumerate-pytest-fixtures).

As a side effect, this patch also includes precise file paths that may be used to reproduce the problem.

Fixes reverbc#20
Fixes reverbc#21

Signed-off-by: Sviatoslav Sydorenko wk@sydorenko.org.ua

Replayed; Source PR: reverbc#22

Additionally, satisfied the https://github.com/pylint-dev/pylint-pytest's .pre-commit-config.yaml

Signed-off-by: Stavros Ntentos 133706+stdedos@users.noreply.github.com

  • Capture and return stdout and stderr (instead of trashing them)
  • Fix the 'duplicate-path' error by relative-izing the paths before unioning them
  • Update tests to test for both conditions

Additionally, fix two used-before-assignment pylint issues (stdout, stderr) 🤪

Signed-off-by: Stavros Ntentos 133706+stdedos@users.noreply.github.com

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3cee43d) 92.25% compared to head (b8f9dc4) 92.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
+ Coverage   92.25%   92.50%   +0.24%     
==========================================
  Files          18       18              
  Lines         542      560      +18     
  Branches      104      109       +5     
==========================================
+ Hits          500      518      +18     
  Misses         29       29              
  Partials       13       13              
Flag Coverage Δ
3.10 92.32% <95.65%> (+0.25%) ⬆️
3.11 92.32% <95.65%> (+0.25%) ⬆️
3.6 92.32% <95.65%> (+0.25%) ⬆️
3.7 92.32% <95.65%> (+0.25%) ⬆️
3.8 92.32% <95.65%> (+0.25%) ⬆️
3.9 92.32% <95.65%> (+0.25%) ⬆️
macos-latest 92.32% <95.65%> (+0.25%) ⬆️
ubuntu-20.04 92.32% <95.65%> (+0.25%) ⬆️
ubuntu-latest 92.32% <95.65%> (+0.25%) ⬆️
windows-latest 92.32% <95.65%> (+0.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
pylint_pytest/checkers/fixture.py 91.15% <100.00%> (+0.24%) ⬆️
tests/test_cannot_enumerate_fixtures.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Should we simply disable the message ? The solution is complex and non obvious to me. Maybe it's a nice check except if you're actually testing pytest fixtures instead of using them in your code ?

@stdedos stdedos added this to the Release v1.1.x milestone Oct 18, 2023
@stdedos
Copy link
Collaborator Author

stdedos commented Oct 18, 2023

I think this is a sanity check whether pytest can process a ficture created at the repository pylint is checking.

I am not sure how much "valuable" it is in CI scenarios (pytest will fail running in that case anyway).
Might be ... for the lazy? A "quick" way to get feedback (given that pylint might run after every file save)?

I haven't dug the git-log for that. We can disable it by default on v2 though?

@Pierre-Sassoulas
Copy link
Member

I meant disable it in pylint-pytest for our own repository, not disable it for users.

@stdedos
Copy link
Collaborator Author

stdedos commented Oct 18, 2023

I meant disable it in pylint-pytest for our own repository, not disable it for users.

I'd ... need to see that when we revisit re-enabling pylint warnings.

... or I didn't understand the question 😓

@Pierre-Sassoulas
Copy link
Member

I'm not sure I understood this MR well. I'm going to focus on compatibility with pylint 3.0 for now (maybe using the addon in pylint itself would help me understand how it works then).

@stdedos stdedos added the bug Something isn't working label Oct 23, 2023
This change applies the pre-existing patterns to identify if the files with collection problems are tests. It is then used to eliminate the false-positives of F6401 (cannot-enumerate-pytest-fixtures).

As a side effect, this patch also includes precise file paths that may be used to reproduce the problem.

Fixes reverbc#20
Fixes reverbc#21

Signed-off-by: Sviatoslav Sydorenko <wk@sydorenko.org.ua>

_Replayed; Source PR: https://github.com/reverbc/pylint-pytest/pull/22_

Additionally, satisfied the https://github.com/pylint-dev/pylint-pytest's `.pre-commit-config.yaml`

Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
* Capture and return `stdout` and `stderr` (instead of trashing them)
* Fix the 'duplicate-path' error by `relative`-izing the paths before `union`ing them
* Update tests to test for both conditions

Additionally, fix two `used-before-assignment` pylint issues (`stdout`, `stderr`) 🤪

Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

LGTM !

@stdedos stdedos merged commit 74589b6 into pylint-dev:master Nov 13, 2023
23 checks passed
@stdedos stdedos deleted the fix/21-exclude-non-tests branch November 13, 2023 10:14
@stdedos stdedos restored the fix/21-exclude-non-tests branch November 13, 2023 12:53
stdedos added a commit that referenced this pull request Nov 20, 2023
* 🐛 Ignore collection failures in non-tests (#15)
* Minor `.github/ISSUE_TEMPLATE/bug_report.md` improvement (22650f9)

Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
@webknjaz
Copy link

@stdedos nice that you picked up my old PR. Too bad that I'm only seeing this now and had the plugin version pinned in my configs for all these years :(

For future, I recommend either re-pushing unmodified commits or at least using the Co-Authored-By: trailers so that the patches are credited correctly: https://hynek.me/til/easier-crediting-contributors-github/. This would've probably notified me earlier too (I think)...

@stdedos
Copy link
Collaborator Author

stdedos commented Apr 24, 2024

Apologies @webknjaz. I did those MRs multiple times.

... I obviously missed something 😅

@webknjaz
Copy link

Yep, it seems like you changed the authorship to Signed-Off-By and dropped the metadata from the commit's built-in fields, losing this information. AFAIK Signed-Off-By should only be added by somebody having a permission from the author as it's used as a DSO in many places, basically as a declaration that the author promises they're in legal possession of a patch being sent. My understanding is that the common recommendations here are the same as for the copyright lines in code — only one adding such a line in the first place is supposed to touch it and other people would need to add their lines separately.
IANAL though, this is my personal understanding of the meaning of such trailers.

@stdedos
Copy link
Collaborator Author

stdedos commented Apr 24, 2024

Yep, it seems like you changed the authorship to Signed-Off-By and dropped the metadata from the commit's built-in fields, losing this information

Basically I'm trying to keep each commit in-line with a Green CI. As CI was done "after" your commit, "I had no option other to re-write it". But ofc I could've kept author/committer and/or Co-authored-by:.


Yeah - in retrospect, I would've done much less: Just pull the commit.

I have done shenanigans to remediate ill-crafted commits (see: 69ad82f) but this MR is literally the first functional change merged. I'm afraid is too late to do something meaningful ❤️

@webknjaz
Copy link

You did a great job resurrecting the project regardless :) Just wanted to log the confusion, that's all.

P.S. Since you mentioned liking evergreen CI and maintenance ergonomics, here's a few of my GitHub Actions that you may enjoy having here:

@webknjaz
Copy link

I'm afraid is too late to do something meaningful ❤️

That's actually fixable by making a true merge commit (not a squash or rebase) from the original branch and having it as this natural merge in master. But it's probably not something to care about.

@stdedos
Copy link
Collaborator Author

stdedos commented Apr 25, 2024

There you go 🙃 8310a88

@webknjaz
Copy link

Yaaaay! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]Why does adding a module produce F6401 0.3.0 found my fixtures; 1.1.2 cannot.
3 participants