Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing exception messages #1080

Closed
cmccandless opened this issue Oct 30, 2017 · 6 comments
Closed

Testing exception messages #1080

cmccandless opened this issue Oct 30, 2017 · 6 comments

Comments

@cmccandless
Copy link
Contributor

Re discussion in #1052, what is the general consensus on testing for exception messages? The way I see it, there are a few ways we can handle this (in order of work required):

  1. Ignore exception messages
  2. Test only for exception messages that are explicitly included in the canonical data for the exercise
  3. When exception messages are not explicitly stated in the canonical data, require that the exercise contributor invent messages for all expected exceptions

Additionally, if we are to test for exception messages at all, we need to have a conventional method of doing so in both Python2 and Python3. assertRaisesRegexp is replaced by assertRaisesRegex in Python3, so as mentioned in #1052 there are a few ways of handling this difference.

@cmccandless
Copy link
Contributor Author

Worth noting is that we have a precedent for methods that differ between Python2 and Python3.

@N-Parsons
Copy link
Contributor

N-Parsons commented Nov 5, 2017

Personally I'm not a great fan of requiring particular error messages to be raised by the user. Meaningful messages are great, but exact wording just feels like unnecessary copy-and-pasting to me, and doesn't give the learner the freedom to think about and express what the meaning/source of the error is. This would particularly frustrate me if I thought that the error message was imprecise or I wanted to also display key values.

If we want to test whether people are including messages with their exceptions, we can do a regex for r".+", which would just test that there is something there, and then trust that people will choose to implement something meaningful. I actually quite like this approach, because it actively encourages something which is good practice, without constraining the learner.

@ilya-khadykin
Copy link
Contributor

ilya-khadykin commented Nov 7, 2017

Usually, there are two issues:

  1. ValueError (or others for that matter) is raised without any meaningful error message which makes the code less readable and hard to use
  2. throwing exceptions of an inappropriate type (it's especially true for Java but it also true for Python as well in some cases)

If we address this two issues this will be a good enough.

@cmccandless
Copy link
Contributor Author

ValueError (or others for that matter) is raised without any meaningful error message which makes the code less readable and hard to use

Sounds like @N-Parsons 's solution of using r".+" would be adequate for this.

throwing exceptions of an inappropriate type (it's especially true for Java but it also true for Python as well in some cases)

This is situational; I don't know that there's a good standardized way to handle this other than please review.

@N-Parsons
Copy link
Contributor

If we do implement checks for the presence of error messages, I think we should definitely add something about it to the README insert so that it's clear that we're expecting them (and to encourage people to make them meaningful).

Exception types are controlled by the test-suite, so there's not really much problem on the learner side, but we should try to have some consistency between the test-suites. We could create something along the lines of the Choosing Exceptions to Raise doc pandas has to give everyone something to refer to.

N-Parsons pushed a commit to N-Parsons/exercism-python that referenced this issue Nov 18, 2017
(Fixes exercism#1080)

* Add self.assertRaisesWithMessage method to relevant exercise tests
    - Uses self.assertRaisesRegex
    - Checks only for the presence of a message, not content
* Add meaningful messages to failing examples
* octal: Switch to using a context manager for exception tests
N-Parsons pushed a commit to N-Parsons/exercism-python that referenced this issue Nov 21, 2017
(Fixes exercism#1080)

* Add self.assertRaisesWithMessage method to relevant exercise tests
    - Uses self.assertRaisesRegex
    - Checks only for the presence of a message, not content
* Add meaningful messages to failing examples
* octal: Switch to using a context manager for exception tests
cmccandless pushed a commit that referenced this issue Dec 12, 2017
* Implement checks for messages being raised with exceptions
(Fixes #1080)

* Add self.assertRaisesWithMessage method to relevant exercise tests
    - Uses self.assertRaisesRegex
    - Checks only for the presence of a message, not content
* Add meaningful messages to failing examples
* octal: Switch to using a context manager for exception tests

* Add note regarding error messages to the insert

* simple-linked-list: Move hints.md to correct location

* simple-cipher: Remove extra whitespace from lines

* collatz-conjecture: Update hints.md

* Regenerate README to include exceptions section
@cmccandless
Copy link
Contributor Author

cmccandless commented Nov 27, 2018

For future record, here is the proper way to handle exception testing in this repository:

class TestClass(unittest.TestCase):
	def test_raises_exception(self):
        with self.assertRaisesWithMessage(ValueError):
            method_that_raises_ValueError()

    # Utility functions
    def setUp(self):
        try:
            self.assertRaisesRegex
        except AttributeError:
            self.assertRaisesRegex = self.assertRaisesRegexp

    def assertRaisesWithMessage(self, exception):
        return self.assertRaisesRegex(exception, r".+")

Edit: if Python 2.7 is no longer supported, the above snippet may be simplified to the following:

class TestClass(unittest.TestCase):
    def test_raises_exception(self):
        with self.assertRaisesWithMessage(ValueError):
            method_that_raises_ValueError()

    # Utility functions
    def assertRaisesWithMessage(self, exception):
        return self.assertRaisesRegex(exception, r".+")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants