prospector icon indicating copy to clipboard operation
prospector copied to clipboard

Mechanical fix/workaround for `tests/tools/pyroma/test_pyroma_tool.py`

Open valeriupredoi opened this issue 1 year ago • 4 comments

Description

This PR provides a mechanical fix/workaround (ie not a fix at the root cause, please see below for reasoning) for the failed test

Testing steps

  • tested locally (on my machine) following the test steps from the Github Action (GA) workflow: result: 100% pass rate
  • tested on GA via my personal fork: I replicated the test failure one can see on the main master

Clues: the files.files are identical on both machines; the messages indeed differ by two elements:

  • Files and messages on local machine:
[PosixPath('/home/valeriu/prospector-fork/tests/tools/pyroma/testdata/pkg1/__init__.py'), PosixPath('/home/valeriu/prospector-fork/tests/tools/pyroma/testdata/pkg1/this_one_is_fine/setup.py'), PosixPath('/home/valeriu/prospector-fork/tests/tools/pyroma/testdata/pkg1/this_one_is_fine/__init__.py')]
[pyroma-PYR05, pyroma-PYR06, pyroma-PYRUNKNOWN, pyroma-PYR09, pyroma-PYR12, pyroma-PYR05, pyroma-PYR06, pyroma-PYRUNKNOWN, pyroma-PYR09, pyroma-PYR12]
  • Files and messages on GA machine:
[PosixPath('/home/runner/work/prospector/prospector/tests/tools/pyroma/testdata/pkg1/__init__.py'), PosixPath('/home/runner/work/prospector/prospector/tests/tools/pyroma/testdata/pkg1/this_one_is_fine/__init__.py'), PosixPath('/home/runner/work/prospector/prospector/tests/tools/pyroma/testdata/pkg1/this_one_is_fine/setup.py')]
[pyroma-PYR05, pyroma-PYR06, pyroma-PYRUNKNOWN, pyroma-PYR09, pyroma-PYR12, pyroma-PYRUNKNOWN, pyroma-PYR05, pyroma-PYR06, pyroma-PYRUNKNOWN, pyroma-PYR09, pyroma-PYR12, pyroma-PYRUNKNOWN]

As you can see, the GA results include two additional pyroma-PYRUNKNOWN members in the messages list. My theory is that the setup.py file gets a little bit altered (trailing space character, newline character etc - could be anything non-major - something minor) which gives differing results at the point of pyroma run. File system peculiarities, I reckon.

PS: I've kept the change to the GA workflow file for possible further testing, I'll remove that if this will get merged. Cheers, fellas :beer:

Related Issue

This failing test needs be fixed so we can get #645 in and consequently release a version that supports Python 3.12

Motivation and Context

Fixes failing test tests/tools/pyroma/test_pyroma_tool.py

How Has This Been Tested?

Types of changes

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [x] My code follows the code style of this project.
  • [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [ ] My change requires a change to the dependencies
  • [ ] I have updated the dependencies accordingly
  • [x] I have read the CONTRIBUTING document.
  • [x] I have added tests to cover my changes.
  • [ ] All new and existing tests passed.

valeriupredoi avatar Apr 04 '24 16:04 valeriupredoi

Thank you for your work @valeriupredoi. I don't have enough knowledge of prospector to review that, sorry.

Pierre-Sassoulas avatar Apr 12 '24 13:04 Pierre-Sassoulas

@Pierre-Sassoulas not a problem at all, mate. Can any other of the prospector core devs review maybe? :beer:

valeriupredoi avatar Apr 15 '24 14:04 valeriupredoi

@carlio would you mind reviewing this to unblock python 3.12 support down the line, please ? :)

Pierre-Sassoulas avatar Apr 15 '24 14:04 Pierre-Sassoulas

am just a bit worried that @carlio 's been off GH scope for many months, do hope he's on a nice sabbatical/holiday :+1:

valeriupredoi avatar Apr 25 '24 12:04 valeriupredoi

@valeriupredoi @Pierre-Sassoulas Forgive me if I am about to say something stupid, i am sailing uncharted water here, but:

Shouldn't this simply be a case of getting the unique values for messages, in this case with a len of 5 (pyroma-PYR05, pyroma-PYR06, pyroma-PYRUNKNOWN, pyroma-PYR09, pyroma-PYR12), and thus satisfying execution in any context?

If order is irrelevant (which i feel it is), messages could be converted into a set and back to a list, assert the len and continue with the iteration.

Thoughts?

robertalexa avatar Sep 27 '24 09:09 robertalexa

Thoughts?

I'm starting to think that this test possibly being invalid is not as impactful than prospector still not being available for python 3.12 / pylint 3.0. (And the numerous CVE it contains from some outdated dependencies)

Pierre-Sassoulas avatar Oct 03 '24 19:10 Pierre-Sassoulas