Repository: pybamm-team/PyBaMM
Pull request #4236 (2024-07-02 18:21:21)

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

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: https://github.com/fossasia/query-server/issues/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?
kratman: I suspect codacy is "happy" because it only fags 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
prady0t: Let me add it back.

kratman: @prady0t Thanks, should be good now
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

Patch:
diff --git a/bandit.yml b/bandit.yml
new file mode 100644
index 000000000..87da61e53
--- /dev/null
+++ b/bandit.yml
@@ -0,0 +1,2 @@
+# To ignore false flagging of assert statements in tests by Codacy.
+skips: ['B101']
diff --git a/pybamm/expression_tree/symbol.py b/pybamm/expression_tree/symbol.py
index df549747c..aa9ebe66d 100644
--- a/pybamm/expression_tree/symbol.py
+++ b/pybamm/expression_tree/symbol.py
@@ -282,7 +282,8 @@ class Symbol:
 
     @name.setter
     def name(self, value: str):
-        assert isinstance(value, str)
+        if not isinstance(value, str):
+            raise TypeError(f"{value} must be of type str")
         self._name = value
 
     @property
diff --git a/pybamm/settings.py b/pybamm/settings.py
index 2ccd9bcd1..d190eaf47 100644
--- a/pybamm/settings.py
+++ b/pybamm/settings.py
@@ -29,8 +29,9 @@ class Settings:
         return self._debug_mode
 
     @debug_mode.setter
-    def debug_mode(self, value):
-        assert isinstance(value, bool)
+    def debug_mode(self, value: bool):
+        if not isinstance(value, bool):
+            raise TypeError(f"{value} must be of type bool")
         self._debug_mode = value
 
     @property
@@ -38,8 +39,9 @@ class Settings:
         return self._simplify
 
     @simplify.setter
-    def simplify(self, value):
-        assert isinstance(value, bool)
+    def simplify(self, value: bool):
+        if not isinstance(value, bool):
+            raise TypeError(f"{value} must be of type bool")
         self._simplify = value
 
     def set_smoothing_parameters(self, k):
diff --git a/pyproject.toml b/pyproject.toml
index 005999d93..2bc6f4f3d 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -200,6 +200,7 @@ extend-select = [
     "UP",            # pyupgrade
     "YTT",           # flake8-2020
     "TID252",        # relative-imports
+    "S101",          # to identify use of assert statement
 ]
 ignore = [
     "E741",        # Ambiguous variable name
@@ -220,7 +221,7 @@ ignore = [
 ]
 
 [tool.ruff.lint.per-file-ignores]
-"tests/*" = ["T20"]
+"tests/*" = ["T20", "S101"]
 "docs/*" = ["T20"]
 "examples/*" = ["T20"]
 "**.ipynb" = ["E402", "E703"]


Test patch:
diff --git a/tests/unit/test_expression_tree/test_symbol.py b/tests/unit/test_expression_tree/test_symbol.py
index 668c07690..e42f8dc8e 100644
--- a/tests/unit/test_expression_tree/test_symbol.py
+++ b/tests/unit/test_expression_tree/test_symbol.py
@@ -18,6 +18,8 @@ import sympy
 class TestSymbol(TestCase):
     def test_symbol_init(self):
         sym = pybamm.Symbol("a symbol")
+        with self.assertRaises(TypeError):
+            sym.name = 1
         self.assertEqual(sym.name, "a symbol")
         self.assertEqual(str(sym), "a symbol")
 
diff --git a/tests/unit/test_settings.py b/tests/unit/test_settings.py
index d70d3d4c4..6573929ad 100644
--- a/tests/unit/test_settings.py
+++ b/tests/unit/test_settings.py
@@ -8,6 +8,9 @@ import pytest
 
 class TestSettings:
     def test_simplify(self):
+        with pytest.raises(TypeError):
+            pybamm.settings.simplify = "Not Bool"
+
         assert pybamm.settings.simplify
 
         pybamm.settings.simplify = False
@@ -15,6 +18,10 @@ class TestSettings:
 
         pybamm.settings.simplify = True
 
+    def test_debug_mode(self):
+        with pytest.raises(TypeError):
+            pybamm.settings.debug_mode = "Not bool"
+
     def test_smoothing_parameters(self):
         assert pybamm.settings.min_max_mode == "exact"
         assert pybamm.settings.heaviside_smoothing == "exact"