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

TestCase.assertRaisesString #123013

Closed
dg-pb opened this issue Aug 14, 2024 · 8 comments
Closed

TestCase.assertRaisesString #123013

dg-pb opened this issue Aug 14, 2024 · 8 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@dg-pb
Copy link
Contributor

dg-pb commented Aug 14, 2024

Feature or enhancement

Proposal:

More often I need to check for exact error message and do not need regular expression.

For pure convenience, this would look like.

def assertRaisesString(self, exc, string, *args, **kwargs):
    escaped = re.escape(string)
    return self.assertRaisesRegex(exc, f'^{escaped}$', *args, **kwds)

However, as using regular expression is overkill, could just add pure string case to _AssertRaisesBaseContext:

class _AssertRaisesBaseContext(_BaseTestCaseContext):

    def __init__(self, expected, test_case, expected_string=None, regex=True):
        ...

class _AssertRaisesContext(_AssertRaisesBaseContext):
    def __exit__(self, exc_type, exc_value, tb):
        expected_string = self.expected_string
        exc_string = str(exc_value)
        if self.regex:
            # perform regex
        else:
            # compare strings

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

Linked PRs

@dg-pb dg-pb added the type-feature A feature request or enhancement label Aug 14, 2024
@Eclips4 Eclips4 added the stdlib Python modules in the Lib dir label Aug 14, 2024
@sobolevn
Copy link
Member

I am a bit sceptical about adding features that only take 3 lines of code to implement in your own code (from your example):

def assertRaisesString(self, exc, string, *args, **kwargs):
    escaped = re.escape(string)
    return self.assertRaisesRegex(exc, f'^{escaped}$', *args, **kwds)

On the other hand, in our own test code this is a very popular pattern.

I would vote for direct string compare, instead of escaped regex match (if this feature is accepted).

I think that adding performance numbers here can help: how fast it is to run a test case with self.assertRaisesRegex(re.escape(...)) and how fast it is to run the same with direct string compare. It might be helpful when there are a lot of such tests.

@picnixz
Copy link
Contributor

picnixz commented Aug 15, 2024

I'm also of the same mind. What I could however suggest is that we extend the unittest.TestCase class by adding test.support.TestCase with some methods like that. It would help us keep the same syntax and we won't need to have to pass self to the function.

@dg-pb
Copy link
Contributor Author

dg-pb commented Aug 15, 2024

To me it probably would be most natural to have only single method assertRaises(exc, expected_string=None, regex=False, callback=None). I quite like APIs where by default it is not regex, but there is a flag for it, such as ripgrep.

However, there is an issue of callback with full *args and **kwds mixed up into the signature.

I used to use similar patterns in the past, but don't do this anymore, instead just have callback argument and if I need args or kwds, I just use partial, otherwise it makes extensions difficult, such as this case.

@serhiy-storchaka
Copy link
Member

There are many more required unittest features on the list. I am skeptical about this one.

@dg-pb
Copy link
Contributor Author

dg-pb commented Aug 15, 2024

Benchmark results (100_000 runs):

regex-convenient: 0.4 s
regex-manual: 0.24 s
string: 0.17 s
change-convenient: -57%
change-manual: -29%

Relative difference is non-trivial, however aggregate impact is small. Could have a slight impact far far in the future, but now it is tiny. CPython - 2041 hits of assertRaisesRegex.

I am +1 for convenience. I think it is useful to have this for CPython tests.

And +0.1 on implementation. Alternatively, could just add 3 lines of Python, but I am still slightly biased towards not computing more than needed given it is a simple addition.

import re, time
import unittest as utt

RNG = list(range(100_000))

class TestCase(utt.TestCase):
    def bench_string(self):
        for i in RNG:
            with self.assertRaisesString(TypeError, 'Error Message'):
                raise TypeError('Error Message')

    def bench_regex_manual(self):
        for i in RNG:
            with self.assertRaisesRegex(TypeError, '^Error Message$'):
                raise TypeError('Error Message')

    def bench_regex_convenient(self):
        for i in RNG:
            msg = 'Error Message'
            msg = re.escape(msg)
            with self.assertRaisesRegex(TypeError, f'^{msg}$'):
                raise TypeError('Error Message')

    def test_bench(self):
        for f in [self.bench_regex_convenient, self.bench_regex_manual, self.bench_string]:
            t = time.perf_counter()
            f()
            print(f'{(time.perf_counter() - t) * 1000:.2f} ms')

if __name__ == '__main__':
    utt.main()

@serhiy-storchaka
Copy link
Member

Usually, it is enough to only test that some words or sequence of words are occurred in the error message, just to differentiate it from other errors with the same type. Such tests are less fragile, you can slightly rewrite the error message without changing tests.

If you need to check the exact error message, it may be better to write it as:

with self.assertRaises(...) as cm:
    ...
self.assertEqual(str(cm.exception), ...)

This works also for warnings, and you can test also other attributes, for example the stack level for warnings.

@dg-pb
Copy link
Contributor Author

dg-pb commented Aug 16, 2024

If you need to check the exact error message, it may be better to write it as:

This is sensible. As long as it is clear that this is the recommended pattern for it, I think this is sufficient.

If everyone is happy with this, I am closing this issue and PR.

@dg-pb
Copy link
Contributor Author

dg-pb commented Aug 16, 2024

.

@dg-pb dg-pb closed this as not planned Won't fix, can't repro, duplicate, stale Aug 16, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Unittest issues Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
Status: Done
Development

No branches or pull requests

5 participants