NCAS-CMS / cf-python

A CF-compliant Earth Science data analysis library
http://ncas-cms.github.io/cf-python
MIT License
120 stars 19 forks source link

Fix misleading error message when it is not possible to create area weights requested from `cf.Field.collapse` #732

Closed davidhassell closed 6 months ago

davidhassell commented 6 months ago

Fixes #731

sadielbartholomew commented 6 months ago

Hi David, did you want me to review this? Asking since it seems you might be waiting on a review but I am not assigned.

davidhassell commented 6 months ago

Hi Sadie - got any ideas on how to test on the value of the exception message string? Is that possible?

sadielbartholomew commented 6 months ago

Hi @davidhassell, the way I am familiar with, and seems quite readable, is to update assertRaises to assertRaisesRegex: see https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaisesRegex. As an example using the test_Field module, this diff works to also test the error message:

diff --git a/cf/test/test_Field.py b/cf/test/test_Field.py
index 6f528642f..255ed45bd 100644
--- a/cf/test/test_Field.py
+++ b/cf/test/test_Field.py
@@ -393,7 +393,9 @@ class FieldTest(unittest.TestCase):
                         data=d,
                     )

-        with self.assertRaises(Exception):
+        with self.assertRaisesRegex(
+            ValueError, "Can't set components=True and data=True"
+        ):
             f.weights(components=True, data=True)

     def test_Field_replace_construct(self):

It would be good to (eventually) go in and update more/most/all error checking to also test the messages in that way: shall I open an Issue for it?

davidhassell commented 6 months ago

+1 - I'll try that

davidhassell commented 6 months ago

Done: 3c2802f42f51c8fad667f257c9c6c4a2dd5f3d19