raise value error from the constructor level

[EricLiclair]

What:

#233

Why:

An error was raised when the comparison of two non-similar type data were compared. To be more technically correct, the error should be raised at the time of creating an instance. And thereby this solution.

How:

Compared the input arguments, passed at the time of constructor call, with the type of class Matcher. If the two type do not match, a Value Error is raised.

Risks:

No Such risks are possible as far as I understand the use of the particular constructors. The only issues am concerned about is that the make install_build_deps does not install typeguard and psutil. That can be a reason for the tests to fail.

Checklist:

• Ensured the test suite passes

• Made sure your code lints

• Completed the Contributor License Agreement ("CLA")

• Added tests, if you've added code that should be tested

• Updated the documentation, if you've changed APIs N/A

Two points to draw attention.

1. The make install_build_deps does not install typeguard and psutil.
2. If ValueError message "Not of type Matcher" is ok?

[coveralls]

Pull Request Test Coverage Report for Build 661743676

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.01%) to 94.141%

---

Totals
Change from base Build 617391924:	0.01%
Covered Lines:	2555
Relevant Lines:	2714

---

💛 - Coveralls

[macisamuele]

@EricLiclair thanks a lot for contributing and for the update.

Could you please add test cases to cover your changes?
It ensures that this PR actually addresses #233 and that no future regressions might happen (at least not un-intentional regression)?

[macisamuele]

nit. I would have more explicit wording in here eventually pointing to what is not a Matcher. Like adding the presentation of a or b.
This would help developers receiving such error to be guided toward what to fix instead of having them discover it themselves

[EricLiclair]

Yes am working on it.

[EricLiclair]

This is how the error message looks.

[macisamuele]

@EricLiclair , sorry for being pedantic ... I hope that you would appreciate it.

I do have some reserves on what the tests are actually trying to test.
My understanding is that the tests (that might benefit from a small re-org, as for the comments) are testing the exact if statements that have been added, but is totally unclear to me how a user would eventually get into that flow. Having this understanding would be beneficial to ensure that wording is consistent and helpful for the framework user.

---

About

The make install_build_deps does not install typeguard and psutil.

This is expected because typeguard and psutil are dependencies of the library and not needed to start the build/test/lint of the library. You should be using make install_deps to install the library dependencies (as it was suggested in here)

[macisamuele]

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)

[EricLiclair]

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.

[macisamuele]

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.