Skip to content

Integration tests should assert the whole API response #438

Description

@mattgoud

Why

The response body is the public contract of the Admin API. A field that is renamed, that
disappears, or that starts being returned when it should not, is a breaking change for
every consumer, and it is invisible in a diff.

Our integration tests are the only thing that can catch it, and today they only do so
when they happen to assert the complete response. Where they assert field by field, the
shape of the payload is unpinned: the test stays green while the contract moves.

This is not written down anywhere, which is why it keeps coming up one review at a time.
So here it is.

The rule

For every endpoint a pull request adds or changes, at least one test must assert the
complete decoded response, not only the fields under test.

The idiom

It already exists in the repository. CustomerGroupEndpointTest::testAddCustomerGroup:

$customerGroup = $this->createItem('/customers/groups', $postData, ['customer_group_write']);
$this->assertArrayHasKey('customerGroupId', $customerGroup);
$customerGroupId = $customerGroup['customerGroupId'];
$this->assertEquals(['customerGroupId' => $customerGroupId] + $postData, $customerGroup);

The expected array is built from the payload that was sent plus the fields the API adds,
then compared to the whole response. AttributeEndpointTest does the same on a PATCH,
and checks that a following GET returns the same thing:

$updatedAttribute = $this->partialUpdateItem('/attributes/attributes/' . $attributeId, $patchData, ['attribute_write']);
$this->assertEquals(['attributeId' => $attributeId] + $patchData, $updatedAttribute);

$attribute = $this->getItem('/attributes/attributes/' . $attributeId, ['attribute_read']);
$this->assertEquals(['attributeId' => $attributeId] + $patchData, $attribute);

Two shapes that look like it but are not enough

Field by field. Every assertion passes while an unexpected field is returned, or a
documented one silently vanishes:

$this->assertSame(200, $updated['width']);
$this->assertTrue($updated['categories']);

Comparing one endpoint's response to another's. This pins consistency between the two,
which is useful, but not the field set. If both sides lose a field, the test stays green:

$this->assertEquals($this->getItem('/currencies/' . $currencyId, ['currency_read']), $created);

Keep them when they help readability. They are not the problem, they are just not the
guarantee we need. Pair them with one assertion on the whole payload.

Where it is already done

This is not a new expectation invented for one contributor, and several recent PRs
already apply it. ImageTypeEndpointTest::testGetImageType spells the nine fields out
and compares them to the whole response. CustomerEndpointTest does it on four of its
CRUD paths. That is exactly the target.

What this is not asking for

  • Not one whole-response assertion per test. One per endpoint is enough, usually on the
    create and on the read.
  • Not the removal of targeted assertions. They document intent and they stay.
  • Not a rewrite of the existing suite. This applies to what a PR adds or touches.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions