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.

Sign up for GitHub

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

Jump to bottom

raise value error from the constructor level #290

Closed
wants to merge 5 commits into from
Closed

raise value error from the constructor level #290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
53afe4a
raise value error from the constructor level
EricLiclair Mar 16, 2021
73ea05d
adding test cases and formatting the error message
EricLiclair Mar 17, 2021
faadb6a
fixinf typo
EricLiclair Mar 17, 2021
494b615
raise value error from the constructor level
EricLiclair Mar 16, 2021
90fc4cd
Merge branch 'master' of https://github.com/EricLiclair/TestSlide
EricLiclair Mar 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions tests/matchers_unittest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,34 @@ def testCannotChainMoreThanTwo(self):
with self.assertRaises(testslide.matchers.AlreadyChainedException):
testslide.matchers.Any() ^ testslide.matchers.AnyStr() ^ testslide.matchers.AnyInt()

def testValueError(self):
# tests for _AndMatcher constructor call
self.assertRaises(ValueError, testslide.matchers._AndMatcher, 10, 30)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few notes around the tests:

  • I would highly recommend to use the assetRaises as context manager such that the code to test is written in a more "natural" form and please assert that the content of the exception matches your expectations (example below). NOTE This applies to all the assertions
  • please create multiple tests with a very scoped objective. In case testValueError would be failing we would have 9 assertions (as for now) to verify and this makes it very hard to scale
  • Even though the unittest might be valid I struggle to match the case to how a user would eventually be able to trigger such error.
    A user of the testing framework should not be using "private" (underscore-prefixed methods/classes) components of the framework. Please create some tests triggering the case that you are covering by this PR.

Example

with self.assertRaisesRegex(ValueError, ${regex matching the string representation of the exception}):
    testslide.matchers._AndMatcher(10, 30)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for your valuable response and yes I do appreciate it. I tried to figure out a way to address the original issue. I believe I understood what needs to be done, but I am kind of stuck as to which all matchers need to be updated. Previously I was changing the private methods _AndMatcher, _OrMatcher and _XorMatcher. But after your last response I kind of believe that those are not where update needs to be done.

Also I tried figuring out what all cases might trigger the error I was trying to raise.

I'm stuck.

I need your help, if you could guide me, where and what all to be updated.

What I have from the past responses:

  1. Use isinstance().
  2. Error to be raised ValueError with an intuitive erro message.
  3. Once updated, add tests for the updated code.
  4. Multiple tests with very scoped objectives.
  5. Natural representation of the test case using assertRaise as context manager.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a small step back and check what the issue reports.
It does mention that if we would be using AnyInstanceOf(not_a_type) it would throw a TypeError because not_a_type is not a type.

>>> isinstance(1, 2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: isinstance() arg 2 must be a type or tuple of types

While it would be better to have a more explicit exception matching that the AnyInstanceOf matcher was not properly configured.

Now, the comments provided so far were a mixture between python-ism and having tests that help us to ensure that what is fixed today does not get broken/removed tomorrow without a direct intent.

What would I expect to see?
Essentially, AnyInstanceOf is used when you want to match parameters into mocked methods.
An example of a "correct" usage would be

class C:
    def foo(self, obj: Any) -> str:
        if isinstance(obj, (str, int)):
            return "string or integer"
        else:
            return str(obj)

class TC(TestCase):
    def test_C_foo(self) -> None:
        c = C()
        self.mock_callable(c, "foo").for_call(AnyInstanceOf(int)).to_return_value("Mocked it properly")
        self.assertEqual(c.foo(1), "Mocked it properly")
        self.assertEqual(c.foo(2), "Mocked it properly")
        with self.assertRaises(UnexpectedCallArguments):
            # This raises because c.foo has no defined behaviour if it gets called with non `int` param
            c.foo("a string")

but now what would happen if I would have a test like

def test_triggering(self) -> None:
    c = C()
    self.mock_callable(c, "foo").for_call(AnyInstanceOf(2)).to_return_value("Canned response")
    c.foo()

It would fail with a very odd exception like

  1) t.TC: test_triggering
    1) TypeError: isinstance() arg 2 must be a type or tuple of types
      File "t.py", line 26, in test_triggering
        c.foo(42)

which makes little sense. Would not it be better to have had something like (fully made up exception here)

  1) t.TC: test_triggering
    1) ValueError: AnyInstanceOf(...) expects a type while an `int` (`2`) was provided
      File "t.py", line 26, in test_triggering
        self.mock_callable(c, "foo").for_call(AnyInstanceOf(2)).to_return_value("Canned response")

When I mentioned

A user of the testing framework should not be using "private" (underscore-prefixed methods/classes) components of the framework. Please create some tests triggering the case that you are covering by this PR.

I meant to create a test like test_triggering that shows how the user would "mis-use" the matchers and that they would now receive the ValuleError exception.

self.assertRaises(
ValueError, testslide.matchers._AndMatcher, 10, testslide.matchers.Matcher()
)
self.assertRaises(
ValueError, testslide.matchers._AndMatcher, testslide.matchers.Matcher(), 10
)

# tests for _XorMatcher constructor call
self.assertRaises(ValueError, testslide.matchers._XorMatcher, 10, 30)
self.assertRaises(
ValueError, testslide.matchers._XorMatcher, 10, testslide.matchers.Matcher()
)
self.assertRaises(
ValueError, testslide.matchers._XorMatcher, testslide.matchers.Matcher(), 10
)

# tests for _OrMatcher constructor call
self.assertRaises(ValueError, testslide.matchers._OrMatcher, 10, 30)
self.assertRaises(
ValueError, testslide.matchers._OrMatcher, 10, testslide.matchers.Matcher()
)
self.assertRaises(
ValueError, testslide.matchers._OrMatcher, testslide.matchers.Matcher(), 10
)


class TestUsageWithPatchCallable(testslide.TestCase):
def test_patch_callable(self):
Expand Down
12 changes: 12 additions & 0 deletions testslide/matchers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ class _AndMatcher(_AlreadyChainedMatcher):
"""

def __init__(self, a: Matcher, b: Matcher) -> None:
if not isinstance(a, Matcher) or not isinstance(b, Matcher):
raise ValueError(
f"Unexpected argument(s) of type '{type(a).__name__}' and '{type(b).__name__}'. Expected 'Matcher' and 'Matcher'."
)
self.a = a
self.b = b

Expand All @@ -65,6 +69,10 @@ def __repr__(self) -> str:

class _XorMatcher(_AlreadyChainedMatcher):
def __init__(self, a: Matcher, b: Matcher) -> None:
if not isinstance(a, Matcher) or not isinstance(b, Matcher):
raise ValueError(
f"Unexpected argument(s) of type '{type(a).__name__}' and '{type(b).__name__}'. Expected 'Matcher' and 'Matcher'."
)
self.a = a
self.b = b

Expand All @@ -90,6 +98,10 @@ def __repr__(self) -> str:

class _OrMatcher(_AlreadyChainedMatcher):
def __init__(self, a: Matcher, b: Matcher) -> None:
if not isinstance(a, Matcher) or not isinstance(b, Matcher):
raise ValueError(
f"Unexpected argument(s) of type '{type(a).__name__}' and '{type(b).__name__}'. Expected 'Matcher' and 'Matcher'."
)
self.a = a
self.b = b

Expand Down