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

add self.assertRaisesWithMessageContaining() to TestBase and use it to clean up test_objects.py #7303

Conversation

Projects
None yet
3 participants
@cosmicexplorer
Copy link
Contributor

commented Mar 2, 2019

Problem

Resolves #7298.

Solution

  • Add self.assertRaisesWithMessage() to TestBase to clearly indicate when an exception message is being tested with self.assertEqual().
    • Also add self.assertRaisesWithMessageContaining() to do a self.assertIn() check.
  • Make test_objects.py uniformly use these methods.

Result

We have nicer test utilities for exception messages, it's more ergonomic to test exception messages exactly, and the testing in test_objects.py is easier to read!

@Eric-Arellano
Copy link
Contributor

left a comment

Yay! Great idea.

Show resolved Hide resolved tests/python/pants_test/test_base.py Outdated
Show resolved Hide resolved tests/python/pants_test/util/test_objects.py
Show resolved Hide resolved tests/python/pants_test/util/test_objects.py
Show resolved Hide resolved tests/python/pants_test/util/test_objects.py

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:add-assert-exception-string-match-method-to-test-base branch from 0f2ddb5 to 947369a Mar 3, 2019

@@ -616,6 +616,23 @@ def assertInFile(self, string, file_path):
content = f.read()
self.assertIn(string, content, '"{}" is not in the file {}:\n{}'.format(string, f.name, content))

@contextmanager
def assertRaisesWithMessageContaining(self, exception_type, error_text, substring_match=False):

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 3, 2019

Contributor

Hm, this new parameter name isn't immediately clear to me what it means. I strongly prefer exact=True, followed by inexact=False instead. If those sound off, maybe exact_message_match or a variant.

I think substring is throwing me off because I always associate a substring with grabbing a specific index from the bigger string and then checking if that specific substring is present.

--

Thanks for changing the default behavior!

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 3, 2019

Author Contributor

My concern was that exact may connote that the alternative is some sort of fuzzy matching. I was also thinking of only_contains. I'll push a commit using exact=True and see what that looks like.

@Eric-Arellano
Copy link
Contributor

left a comment

It looks good to me! Let’s see what others think about the parameter exact.

@illicitonion

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

This generally looks great, thanks!

I would be in favour of dropping exact and just having two methods:

assertRaisesWithMessageContaining and assertRaisesWithMessage (or assertRaisesWithExactMessage)

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

Great suggestion @illicitonion! I agree two methods is the best way to approach this.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:add-assert-exception-string-match-method-to-test-base branch from cabd414 to ba36feb Mar 3, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2019

I have split the method into two: self.assertRaisesWithMessage() and self.assertRaisesWithMessageContaining(). I thought it was a good idea to make the less complicated method name (and the one that seems like the more "normal" behavior) into the one we'd prefer people to use, and then make the self.assertIn() check an explicit note that it's checking whether a string is contained, since overbroad self.assertIn() checks have led to errors in this repo before (as @Eric-Arellano was alluding to above).

@illicitonion
Copy link
Contributor

left a comment

Thanks!

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:add-assert-exception-string-match-method-to-test-base branch from ba36feb to cd644e2 Mar 3, 2019

@cosmicexplorer cosmicexplorer merged commit 1b4f534 into pantsbuild:master Mar 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.