Skip to content

Commit

Permalink
(NFC) dev/core#2029 - Make assertions in PrevNextTest more skimmable
Browse files Browse the repository at this point in the history
Overview
--------

The test appears to have a random failure. Making it more readable may help figure out why.

Before
------

`testDeleteByCacheKey()` and the related `testFillArray()` both have some assertions in these two forms:

```
// Form #1, more verbose
$all = ...getSelections($cacheKey, $action);
$this->assertEquals([...expected...], array_keys($all)...);

// Form #2, more pithy
$this->assertSelections([...expected...], $action, $cacheKey);
```

After
-----

The verbose form is replaced with the pithier form.

In the pithier form, some of the default inputs are made explicit.

Comment
-------

It is confusing that the method `getSelections()` has a parameter `$action`
which can be `get` or `getall` -- and that `getall` does not (in fact) return
selections.  It returns all!  (The fact that the contract is weird makes the
unit-test helpful imho...)
  • Loading branch information
totten committed Oct 21, 2020
1 parent eba889b commit 3e9cd8a
Showing 1 changed file with 9 additions and 12 deletions.
21 changes: 9 additions & 12 deletions tests/phpunit/E2E/Core/PrevNextTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,10 @@ public function testFillArray() {
$this->assertEquals(4, $this->prevNext->getCount($this->cacheKey));
$this->assertEquals(0, $this->prevNext->getCount('not-a-key-' . $this->cacheKey));

$all = $this->prevNext->getSelection($this->cacheKey, 'getall')[$this->cacheKey];
$this->assertEquals([100, 400, 200, 300], array_keys($all));
$all = $this->assertSelections([100, 400, 200, 300], 'getall', $this->cacheKey);
$this->assertEquals([1], array_unique(array_values($all)));

$this->assertSelections([]);
$this->assertSelections([], 'get', $this->cacheKey);
}

public function testFetch() {
Expand Down Expand Up @@ -250,20 +249,15 @@ public function testDeleteByCacheKey() {
// Add some data that we're actually working with.
$this->testFillArray();

$all = $this->prevNext->getSelection($this->cacheKey, 'getall')[$this->cacheKey];
$this->assertEquals([100, 400, 200, 300], array_keys($all), 'selected cache not correct for ' . $this->cacheKey
. ' defined keys are ' . $this->cacheKey . 'and ' . $this->cacheKeyB
. ' the prevNext cache is ' . print_r($this->prevNext, TRUE)
);
$all = $this->assertSelections([100, 400, 200, 300], 'getall', $this->cacheKey);

list ($id1, $id2, $id3) = array_keys($all);
$this->prevNext->markSelection($this->cacheKey, 'select', [$id1, $id3]);
$this->assertSelections([$id1, $id3]);
$this->assertSelections([$id1, $id3], 'get', $this->cacheKey);

$this->prevNext->deleteItem(NULL, $this->cacheKey);
$all = $this->prevNext->getSelection($this->cacheKey, 'getall')[$this->cacheKey];
$this->assertEquals([], array_keys($all));
$this->assertSelections([]);
$this->assertSelections([], 'getall', $this->cacheKey);
$this->assertSelections([], 'get', $this->cacheKey);

// Ensure background data was untouched.
$this->assertSelections([100], 'get', $this->cacheKeyB);
Expand Down Expand Up @@ -329,6 +323,8 @@ public function testDeleteAll() {
* Contact IDs that should be selected.
* @param string $action
* @param string|NULL $cacheKey
* @return array
* Contact IDs that were returned by getSelection($cacheKey, $action)
*/
protected function assertSelections($ids, $action = 'get', $cacheKey = NULL) {
if ($cacheKey === NULL) {
Expand All @@ -342,6 +338,7 @@ protected function assertSelections($ids, $action = 'get', $cacheKey = NULL) {
);

$this->assertCount(count($ids), $selected);
return $selected;
}

}

0 comments on commit 3e9cd8a

Please sign in to comment.