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

One assertion per test #5

Open
bhardin opened this issue Dec 21, 2017 · 5 comments
Open

One assertion per test #5

bhardin opened this issue Dec 21, 2017 · 5 comments

Comments

@bhardin
Copy link
Contributor

bhardin commented Dec 21, 2017

Discussion on best practice of One assertion per test.

@pedroceles
Copy link

@bhardin I think sometimes, specially when testing attribute values we might relax this rule.

Suppose I have the following code (kind of pseudo-code):

class WeirdCertificate:
    id = some_uuid
    stakeholder = 1
    quantity = 3
    uncopied_field = "foo"

    def copy(self):
        new_cert = WeirdCertificate(quantity=self.quantity, stakeholder=self.stakeholder)
        return new_cert

I want to test that copy copies some (but not all) attributes of my class. I can see some approaches to that:

  1. Make multiple tests for each attribute (IMHO, unnecessarily verbose and repetitive).
  2. Make one assertion per attribute in a single test. (breaks the rule. However in this case, I see no problem. To make the test more readable the assertions could be transferred to a helper method right below it).
  3. Make one assertion by joining the boolean comparisons in multiple ands. Like:
    assert (old_cert.quantity == new_cert.quantity) and (old_cert.stakeholder == new_cert.stakeholder)
    (Follows the rule, but the test becomes extremely ugly, and the error message will be very difficult to read).
  4. Captures the desired attributes in a comparable data structure, like a (named)tuple and compare them.
    old_tuple = (old_cert.quantity, old_cert.stakeholder)
    new_tuple  = (new_cert.quantity, new_cert.stakeholder)
    assert old_tuple == new_tuple
    (Follows the rule, however in a way we are still doing multiple asserts. We're jut leveraging the assert api to be used with tuples.)

I'd like to know if any of the alternatives to the first suggestion is a valid one. Or if there is any other way we could test this in a single test. Because I honestly think it is a brutal overkill to make a test per attribute.

@jethrolaicarta
Copy link

I agree with @pedroceles. I think this rule will add a lot of overhead and repetition in many cases.

@levinsamuel
Copy link

levinsamuel commented Jun 4, 2020

Another time I have multiple assertions is: sometimes, I want to test a result, but that result only makes sense or is only valid if another condition is also true. Sometimes I'll do a preliminary assertion to make sure the test is actually testing something, particularly with fixtures where initializing it is out of the scope of the test.

class TestComplexMethod:
    # Fixture abstracts creation of a complex input structure
    def test_complex_method_result_in_attribute_case(self, complex_input):
        # assert that input has a certain quality without which this test is invalid
        assert complex_input.attribute
        result = complex_method(complex_input)
        assert result == 4

@levinsamuel
Copy link

Basically I agree with the ethic of this rule but also think it's important to consider this a guideline more than a rule.

@pedroceles
Copy link

One other good practice that I agree, and it is exemplified in "Clean Code". Is to assert only one thing. But that might now mean using assert only once in all cases. But we can create an "assert method" like:

def test_responds_a_list(self):
    response = view(request)
    self._assert_responds_a_list(response)

def _assert_responds_a_list(self, response):
    assert response.status_code == 200
    assert response.data == []

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

No branches or pull requests

4 participants