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

Improved test assertion method #13

Merged
merged 1 commit into from Jul 8, 2019

Conversation

rennerocha
Copy link

When a test fails, there are no clear indication of the input strings used for the test. This makes identifying which test case is failing very difficult.

Current:

    def test_parsing(example: Example):
        parsed = Price.fromstring(example.price_raw, example.currency_raw)
>       assert parsed == example
E       AssertionError: assert Price(amount=None, currency='EUR') == Example(amount=None, currency=None)

After this PR:

    def test_parsing(example: Example):
        parsed = Price.fromstring(example.price_raw, example.currency_raw)
>       assert parsed == example, f"Failed scenario: price={example.price_raw}, currency_hint={example.currency_raw}"
E       AssertionError: Failed scenario: price=SOMETHING EUROPE SOMETHING, currency_hint=None
E       assert Price(amount=None, currency='EUR') == Example(amount=None, currency=None)

@codecov
Copy link

codecov bot commented Jul 5, 2019

Codecov Report

Merging #13 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master    #13   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          93     93           
  Branches       21     21           
=====================================
  Hits           93     93

@@ -1974,7 +1974,7 @@ def __eq__(self, other):
)
def test_parsing(example: Example):
parsed = Price.fromstring(example.price_raw, example.currency_raw)
assert parsed == example
assert parsed == example, f"Failed scenario: price={example.price_raw}, currency_hint={example.currency_raw}"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be better to use pytest ids for this - see https://docs.pytest.org/en/latest/example/parametrize.html#different-options-for-test-ids. We can use Example as an id; this would also allow to have more readable test names for tests which are passing.

Copy link
Author

Choose a reason for hiding this comment

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

The representation of Example is useless. As it inherits from Price, its string representation doesn't contains the input arguments to fromstring method. IMO this is the most important information that is missing when our tests fails.

This is what we get from Example repr:

ipdb> repr(value)                                                                                                                                                           
"Example(amount=Decimal('12.99'), currency='US$')"

As you can see, it shows nothing about the the input US$:12.99.

But these are the information that we really need when reading the tests fails:

ipdb> value.currency_raw, value.price_raw                                                                                                                                   
(None, 'US$:12.99')

We could create an id based on the raw values. It is better than the actual error, but as we have limitations over the characters that can be user as an id (considering that the inputs has spaces and special characters), it won't contain more detailed information about the inputs.

Another option I can see is to define __repr__ of Example class, so it returns all values provided.

However, I believe that the only values that really matter when we see an error are the ones passed to .fromstring method.

Copy link
Member

Choose a reason for hiding this comment

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

ok, fair enough, thanks for the explanation @rennerocha!

@kmike kmike merged commit 683633a into scrapinghub:master Jul 8, 2019
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

Successfully merging this pull request may close these issues.

None yet

2 participants