#2984 Fix python version test to support python 3.10

[hiker]

Fixes #2984 (and a minor pylint warning)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.90%. Comparing base (4a10286) to head (a8f9279).
Report is 411 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2986   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files         365      365           
  Lines       51586    51586           
=======================================
  Hits        51538    51538           
  Misses         48       48           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[hiker]

Ready for review - the doc failure is an external link to https://gtr.ukri.org/projects?ref=NE%2FL01209X%2F1 timing out.

[LonelyCat124]

Hi @hiker - thanks for fixing this. I've checked that its all working correctly with the changes for 3.9 and 3.10 (provided sphinx is installed).
While we're modifying this - could I get you to add a check for if sphinx is installed (e.g. sys.modules.get("sphinx", None) to skip the type checking tests in this section of tests (i.e. lines 560 to 585), or to do the alternate output (i.e. similar to what we have for pre-3.10 for the second test). These tests currently break without sphinx installed.

I'm personally happy with the former as our test suite runs with sphinx installed now - @sergisiso or @arporter any preference?

[arporter]

I think we've accepted that sphinx is now a dependence for the test suite so I'm happy to stick with that.

[sergisiso]

I know this was discussed in the kwargs PR but I lost track of it before I was merged, why is shpinx needed for the tests? I assume because the type stringify is part of an error message? I still see this as problematic because presumably the production systems won't install it (e.g. spack) and it means that what we test is different that when people will run. I wouldn't use sphinx for the tests if possible.

I am ok using it to improve the generated docstring for the inherited kwargs since we already need sphinx to generate the docs anyway.

[LonelyCat124]

I know this was discussed in the kwargs PR but I lost track of it before I was merged, why is shpinx needed for the tests? I assume because the type stringify is part of an error message? I still see this as problematic because presumably the production systems won't install it (e.g. spack) and it means that what we test is different that when people will run. I wouldn't use sphinx for the tests if possible.

My argument for using sphinx for the test suite is because the thing we want to test here really is that we get the documentation that we expect to see for the docs, but I could be convinced either way.

[sergisiso]

Oh, I see, and it is probably a coverage issue not testing it. I am ok, with:

"sys.modules.get("sphinx", None) to skip the type checking tests in this section of tests (i.e. lines 560 to 585)

But I wouldn't want that we inadvertedly rely on some sphinx behaviour, maybe one of the three pytest can be without sphinx? I see the coverage comes from 3.8, so leave sphinx in that one?

[LonelyCat124]

There are some tests elsewhere that force switch off Sphinx already to test imports are correct, and some other tests that unit test the PSyclone function that hides behind those imports.

[arporter]

My apologies but I've belatedly realised that my #3034 makes this one unnecessary. Does any of the discussion about Sphinx need to be taken up in #3034?

[LonelyCat124]

My apologies but I've belatedly realised that my #3034 makes this one unnecessary. Does any of the discussion about Sphinx need to be taken up in #3034?

Probably yes - though I think there are some tests in psyGen_test and docstring_parser_test that pretend sphinx isn't available. so it depends if those are sufficient for what we had discussed here.

[sergisiso]

I still have some reservations about the Sphinx situation, but since I raised these comments here, I am also happy to close them. If I think this is worth solving I will open a proper ticket, I am usure for now.