fix fixture checker

[anis-campos]

the current implementation provokes recursion errors because of the
VariablesChecker.add_message patch not working properly.
This rework fix the issue by replacing the original variable checker by a subclass, without patch.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bb1a8de) 93.98% compared to head (46cf450) 98.02%.
Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
+ Coverage   93.98%   98.02%   +4.03%     
==========================================
  Files          18       20       +2     
  Lines         549      556       +7     
  Branches      106      105       -1     
==========================================
+ Hits          516      545      +29     
+ Misses         23        8      -15     
+ Partials       10        3       -7     

Flag	Coverage Δ
3.10	98.02% <100.00%> (+4.03%)	⬆️
3.11	98.02% <100.00%> (+4.03%)	⬆️
3.12	98.02% <100.00%> (+4.03%)	⬆️
3.8	98.02% <100.00%> (+4.03%)	⬆️
3.9	98.02% <100.00%> (+4.03%)	⬆️
macos-latest	98.02% <100.00%> (+4.03%)	⬆️
ubuntu-latest	97.66% <100.00%> (+3.67%)	⬆️
windows-latest	98.02% <100.00%> (+4.03%)	⬆️

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

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

[stdedos]

the current implementation provokes recursion errors because of the
VariablesChecker.add_message patch not working properly.

Would you mind creating a test, that demonstrates the issue?

[anis-campos]

the current implementation provokes recursion errors because of the
VariablesChecker.add_message patch not working properly.

Would you mind creating a test, that demonstrates the issue?

I would love to, but i'm not sure how.

I dived on this a bit, turns out the issue is invisible in the tests suits because we are running mono process checks. In our project, we use pylint -j 0 that will run the checks in several threads, one per cpu cores.

This means that I'm actually fixing a thread safety issue.

Now, that being said, I would like to reproduce it here, but I'm not sure how to make a test suit that will use a the -j option of pylint, as we are not actually running pylint, just unit checking the checkers.

Should we include a new test folder, that would actually run pylint , something akin to a integration_tests ?

[anis-campos]

I went with a very simple integration test. This run https://github.com/pylint-dev/pylint-pytest/actions/runs/7310392985 shows the error before the fix

[anis-campos]

@stdedos , happy new year 🎊. Can we go forward with this PR ?

[stdedos]

Hello @anis-campos! Happy new year 🎊🎊🎊

Apologies for the long radio silence. Winter vacations were not ... so much vacations for me, sadly 😅

Thank you for adding the test. While I don't understand "exactly" what is the issue - it is clearly an issue, and one your commit deals with!

The b52f6f6 (#29) commit is also definitely wanted. I'd stamp it immediately, if it were separate.

AFAIS, the essence of your first commit is:

pylint_pytest/__init__.py

• Make register explicit, rather than implicit/recursive into explicit. 👍 / 👎
  There are both arguments here. I'd say it's nicer not to have to remember to register your checker (ofc, "how many times are you going to add new checkers?" is also a question ...)
• ... and remove_original_variables_checker(). If we have to do it, we do it 👍

pylint_pytest/checkers/variables.py

• You essentially moved all patch_add_message logic into the new CustomVariablesChecker. 👍

pylint_pytest/checkers/fixture.py

• ... by doing this, you avoided all of the def open(self) fiddling?? 👍 👍 👍 (You should then be able to remove replacement_add_message? I think it's unused at this point. What about self.checker.open() calls?)
• You renamed a bunch of FixtureChecker's "private" class variables into public.
  I assume you need to access FixtureChecker.pytest_fixtures from CustomVariablesChecker without pylint complaining?
  If we have to do it, we do it 👍. I'd like to avoid "exposing internals" if not necessary, though 👎 Spraying with # pylint: disable=protected-access is acceptable (I'm thinking to experiment with exposing less at a later time)

[anis-campos]

There are both arguments here. I'd say it's nicer not to have to remember to register your checker (ofc, "how many times are you going to add new checkers?" is also a question ...)

that was indeed my reasoning, I wasn't sure it it was worth it trying to understand the auto discovery when there is only 3 classes to register.

If we have to do it, we do it 👍. I'd like to avoid "exposing internals" if not necessary, though 👎 Spraying with # pylint: disable=protected-access is acceptable (I'm thinking to experiment with exposing less at a later time)

Hum, it is still a internal use only, so yes, we can keep it private.

[stdedos]

Amazing MR 🤩

I am deeply sorry this has taken so much time to go through 🙏