MAINT: Consider warnings as errors when running pytest

[jhlegarreta]

Consider warnings as errors when running pytest.

[effigies]

I would suggest not adding -Werror for the min runs. Warnings are expected with old versions.

[jhlegarreta]

Tried different things here:

• aeacf9c: Adding --strict-warnings to [tool.pytest.ini_options] addopts did not work as the flag seemed not be understood by pytest:
  https://github.com/nipreps/nireports/actions/runs/12700037859/job/35401948885#step:12:42
• ed057ad: Adding -W error to [tool.pytest.ini_options] addopts results in pytest not bypassing the DeprecationWarning filtering rule in pyproject.toml and exiting without running all tests:
  https://github.com/nipreps/nireports/actions/runs/12700533251/job/35403381323
• aa05a60 : Adding -W error to the pytest command call in tox.ini results also in pytest not bypassing the DeprecationWarning filtering rule in pyproject.toml and exiting without running all tests.
  https://github.com/nipreps/nireports/actions/runs/12700597202/job/35403576905
• Ended up resorting to the ENH: Consider warnings as errors when running pytest nifreeze#40 solution: pytest fails with some other error
  https://github.com/nipreps/nireports/actions/runs/12700637456/job/35403696969#step:12:439

So ran out of ideas.

As for

I would suggest not adding -Werror for the min runs. Warnings are expected with old versions.

If you have a suggestion to avoid applying the option for them, I would be happy to add it.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.75%. Comparing base (4b9a13b) to head (2859470).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #157      +/-   ##
==========================================
+ Coverage   66.45%   66.75%   +0.29%     
==========================================
  Files          27       27              
  Lines        3023     3023              
  Branches      401      401              
==========================================
+ Hits         2009     2018       +9     
+ Misses        897      892       -5     
+ Partials      117      113       -4     

Flag	Coverage Δ
unittests	62.65% <100.00%> (?)

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.

🚀 New features to boost your workflow:

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

[effigies]

You could do something like this in tox.ini:

command =
    pytest ... \
    !min:  -Werror \
      {posargs:-n auto}

[effigies]

This warning setting takes precedence over those in tool.pytest.ini_options. For min, we expect a lot of warnings that the older versions of the libraries we're testing of course cannot resolve. For pre, we expect many PendingDeprecationWarnings and DeprecationWarnings before our upstreams even have time to address.

[oesteban]

Added to discussion for next TechMon

[jhlegarreta]

@effigies 8744db8 is the best solution I could come up with after many trial and error attempts; the thing is working locally for me. CIs show the right command:
https://github.com/nipreps/nireports/actions/runs/13276532890/job/37067053254?pr=157#step:14:40

py310-latest: commands[0]> pytest --durations=20 --durations-min=1.0 --cov-report term-missing -Werror -n auto

and
https://github.com/nipreps/nireports/actions/runs/13276532890/job/37067054313?pr=157#step:14:40

py39-min: commands[0]> pytest --durations=20 --durations-min=1.0 --cov-report term-missing -n auto

but py310 is failing for another reason and stopping all other executions. The issue is being raised as a warning on main:
https://github.com/nipreps/nireports/actions/runs/13276183449/job/37066108558#step:14:339

Do not have a fix for that.

The issue with the e.g. solution in a73c68f seems to be that {!min:-Werror} is being treated as a string literal and making the pytest calls fail.

[jhlegarreta]

The solution in 4de5fca stops all jobs whenever a warning is encountered. Ideally, we would like all CIs to run and finish independently and only then report warnings. This can be achieved if the solution in nipreps/nifreeze#40 is adopted, so I'd vote for adopting it here.

BTW, the Python 3.9 min build is raising some warnings that only appear in that build:
https://github.com/nipreps/nireports/actions/runs/13577984850/job/37958440074#step:14:488
and some of them having been already addressed, e.g.
https://github.com/nipreps/nireports/actions/runs/13577984850/job/37958440074#step:14:514
addressed in #156. At least they were for the rest of the builds, but persisted for Python 3.9 min: https://github.com/nipreps/nireports/actions/runs/13215907770/job/36895122524#step:12:374.

Similarly, Python 3.11 is treating an already fixed warning as an error in this PR:
https://github.com/nipreps/nireports/actions/runs/13578088021/job/37958736735?pr=157#step:14:331
which was addressed in #166.

These two things are weird to me.

[jhlegarreta]

So compared to 4de5fca, ebc9d39 allows all jobs to complete, instead of cancelling the rest of the jobs when one of them fails, so it looks a better solution to me. Now,

• Python 3.9 min is the only build containing warnings, but warnings are not raising an error. The proposed solution was working for NiFreeze, so not sure what's wrong in here.
• As noted in MAINT: Consider warnings as errors when running pytest #157 (comment), some of the warnings in Python 3.9 min had already been addressed. They seem to still have been present in the Python 3.9 min builds.

A review from somebody else would be greatly appreciated.

[jhlegarreta]

In 1932993 I deliberately added a statement that would raise a warning to check whether the proposed solution would work for the rest of the builds. And the solution does work:
https://github.com/nipreps/nireports/actions/runs/13616720772/job/38060747582?pr=157#step:14:474
https://github.com/nipreps/nireports/actions/runs/13616720772/job/38060748121?pr=157#step:14:479

Other CI jobs still get cancelled when some of the builds errors (the conclusion in #157 (comment) was thus inaccurate, based on the fact that all were passing), but it looks like that is not the most pressing problem now.

The worrying parts are thus:

• The proposed solution that adds the machinery in conftest.py to read the warnings and flag them as errors works for all builds except for Python 3.9 min. Not sure why.
• There Python 3.9 min build has warnings that are solved for other builds in a very generic way (i.e. with no conditional statements about the Python version in the code) and some other Python 3.9 min-specific warnings. Filtering the latter does not seem a good solution, as the filter would be global (i.e. applied to all builds).

[jhlegarreta]

@effigies Thanks for reviving this. I rebased on main and removed what I had added to test that the warnings are considered as errors:

and it is doing its job as we've seen:

So it should be good to go now.

Edit: the comments in #157 (comment) are no longer relevant as Python 3.9 has been dropped.

[jhlegarreta]

Sorry, run out of ideas to fix this:

  nireports/tests/test_reportlets.py::test_plot_raincloud[False-h]
  nireports/tests/test_reportlets.py::test_plot_raincloud[True-h]
  nireports/tests/test_reportlets.py::test_plot_raincloud[False-v]
  nireports/tests/test_reportlets.py::test_plot_raincloud[True-v]
    /home/runner/work/nireports/nireports/nireports/reportlets/nuisance.py:1108: SettingWithCopyWarning: 
    A value is trying to be set on a copy of a slice from a DataFrame
    
    See the caveats in the documentation: https://pandas.pydata.org/pandas-docs/stable/user_guide/indexing.html#returning-a-view-versus-a-copy
      df_clip[feature] = df_clip[feature].clip(lower=lower_limit_value, upper=upper_limit_value)

I cannot reproduce it locally.