amitaibu / og

A fork to work on OG8
https://github.com/Gizra/og
29 stars 16 forks source link

GroupManagerTest is using Prophecy incorrectly #210

Open pfrenssen opened 8 years ago

pfrenssen commented 8 years ago

According to the documentation of the Prophecy mocking framework if you do repeated calls to the same method with identical arguments, then the same result will always be returned (see the section Method prophecies idempotency).

In GroupManagerTest we are violating this principle repeatedly. We're doing multiple calls to Config::get('groups') but we are expecting different results to be returned from it. An example:

  public function testRemoveGroup() {
    // ...
    $groups_before = ['test_entity' => ['a', 'b']];
    $this->configProphecy->get('groups')
      ->willReturn($groups_before)
      ->shouldBeCalled();

    // ...
    $groups_after = ['test_entity' => ['a']];
    $this->configProphecy->get('groups')
      ->willReturn($groups_after)
      ->shouldBeCalled();

    // ...
  }

In reality the $groups_before value will never be returned, instead the $groups_after value will always be returned.

The test currently only works because the initial values that are returned by the mock are not actually used anywhere in the code.

We should fix this according to the "promises" example that is mentioned in that section of the Prophecy documentation.

pfrenssen commented 8 years ago

Here is an example of how this can be fixed: https://github.com/amitaibu/og/pull/192/commits/58dc2cca84c64e4d5e2653824e88abeb752eb33e