Removing false flagging of assert statements from tests.

[prady0t]

Codacy keeps flagging assert statements from tests. This added rule solves this.

[kratman]

Doesn't this ignore all asserts? We do not want asserts in the regular codebase.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.45%. Comparing base (27c29df) to head (daf3918).
Report is 211 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4236   +/-   ##
========================================
  Coverage    99.45%   99.45%           
========================================
  Files          288      288           
  Lines        22086    22089    +3     
========================================
+ Hits         21966    21969    +3     
  Misses         120      120           

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

[agriyakhetarpal]

Doesn't this ignore all asserts? We do not want asserts in the regular codebase.

Yes, it does and we discussed this – I don't think there is a way to configure it per path, and many other projects have the same issue: fossasia/query-server#332. We can look at assert statements in PRs manually though, even though that means having more work on our end.

[kratman]

Is codacy even doing anything for us? Can we just make ruff stricter?

[agriyakhetarpal]

@valentinsulzer brought up the idea, and yes, we can do it because Ruff has the same rules available: https://docs.astral.sh/ruff/rules/#flake8-bandit-s

[kratman]

I do think it is good to check for complexity (which codacy does), and asserts should be avoided in the main codebase. I think there is a lot we could improve by ramping up the ruff strictness. I think it should probably get a lot stricter based on the warnings I see in PyCharm

[kratman]

@prady0t Looks like a good improvement to me. It caught asserts in main code as well. We probably want to raise exceptions or log warnings instead.

[kratman]

I think from here we could try suppressing the asserts in codacy (if that does not cause issues in ruff's analysis) then just rely on ruff for most of it. Can you confirm that ruff triggers while the bandit yaml file suppresses codacy?

[agriyakhetarpal]

I think from here we could try suppressing the asserts in codacy (if that does not cause issues in ruff's analysis) then just rely on ruff for most of it. Can you confirm that ruff triggers while the bandit yaml file suppresses codacy?

@kratman, I think it does, I see Codacy is happy now and doesn't flag the assert statements in tests/, while Ruff caught the ones to catch in pybamm/. I shall approve this PR; IMO, we should add the rest of the flake8-bandit rules that Codacy uses to Ruff separately from here so that we can continue to use Codacy for now, and to let these changes be helpful for other PRs where tests are being modified.

[kratman]

I suspect codacy is "happy" because it only flags new findings. No new asserts were added in the tests here. I think Codacy is still going to have findings when you add new PyTest code. That is why I was asking @prady0t if the bandit yaml file could still be added while adding the ruff changes.

[kratman]

In my local tests the bandit.yml file does not seem to stop ruff from finding asserts in the main code, so we should add that back

[agriyakhetarpal]

In my local tests the bandit.yml file does not seem to stop ruff from finding asserts in the main code, so we should add that back

But that's what we were wanting to do, right? i.e., have Ruff find asserts through its own rules in pyproject.toml, but let Codacy silence itself through bandit.yml – Ruff won't read bandit.yml.

I suspect codacy is "happy" because it only flags new findings. No new asserts were added in the tests here. I think Codacy is still going to have findings when you add new PyTest code. That is why I was asking @prady0t if the bandit yaml file could still be added while adding the ruff changes.

I see. Yes, I've seen Codacy flagging just new findings too earlier. @prady0t, could you push a dummy commit here where you add an assert statement somewhere, one each for the code and the tests, and we can check if Codacy flags those? Ruff seems to be working, I guess, because it flagged the ones in the code but left the ones in the tests (we check with --all-files in the style job).

[kratman]

@agriyakhetarpal, @prady0t That is what I am pointing out. The bandit.yml file was remove from this PR. It should be put back otherwise codacy will continue to flag pytest updates

Never mind – thanks to @kratman for pointing out. We need the bandit.yml file that was added and then removed here to be added again.

[prady0t]

Let me add it back.

[kratman]

@prady0t Thanks, should be good now

[agriyakhetarpal]

Coverage seems to drop because the in-line assert tests will now need separate test cases

[prady0t]

Let me add tests

[kratman]

@prady0t While you are at it, can you add the type hints to the function call?

[kratman]

Tests were passing before along with coverage, so I will merge this shortly

[prady0t]

Thanks @kratman

[agriyakhetarpal]

@prady0t, I was able to find a better fix for this – could you open a PR?

The fix is to add

assert_used:
  skips: ['*_test.py', '*test_*.py']

in bandit.yml, through which we can let Codacy ignore the assert statements selectively and keep the tool enabled till the time we are able to make Ruff implement all of the options.