Skip to content

Commit fc6564b

Browse files
simonLeary42simonLeary42
authored
use assert() instead of phpunit assertTrue() (#357)
1 parent cc1cf35 commit fc6564b

13 files changed

+97
-67
lines changed

‎.pre-commit-config.yaml‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,10 +103,19 @@ repos:
103103
entry: bash ./test/assert-utils-used.bash
104104
language: system
105105
files: \.php$
106-
exclude: ^resources/lib/utils\.php$
106+
exclude: |
107+
(?x)^(
108+
test/.*|
109+
resources/lib/utils\.php|
110+
)$
107111
- id: assert-exceptions-used
108112
name: Assert exceptions are used
109113
entry: bash ./test/assert-exceptions-used.bash
110114
language: system
111115
files: ^resources/.*\.php$
112116
exclude: ^resources/lib/UnityHTTPD\.php$
117+
- id: assert-no-assertTrue
118+
name: Assert `assertTrue` is not used
119+
entry: bash ./test/assert-no-assertTrue.bash
120+
language: system
121+
files: ^test/.*\.php$

‎CONTRIBUTING.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
This will enable strict mode and throw an exception rather than returning `false`.
2323
- `UnityHTTPD`'s user-facing error functionality (ex: `badRequest`) should only be called from `webroot/**/*.php`.
2424
`resources/**/*.php` should throw exceptions instead.
25+
- No code should call `PHPUnit\Framework\TestCase::assertTrue`, instead `assert`.
26+
`assert` is better because it doesn't just say "something is false", it actually shows what is false.
2527

2628
This repository will automatically check PRs for linting compliance.
2729

‎test/assert-no-assertTrue.bash‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
set -euo pipefail
2+
trap 's=$?; echo "$0: Error on line "$LINENO": $BASH_COMMAND"; exit $s' ERR
3+
if [[ $# -lt 1 ]]; then
4+
echo "at least one argument required"
5+
exit 1
6+
fi
7+
8+
rc=0
9+
# --color=never because magit git output log doesn't support it
10+
grep_rc=0; grep -H --color=never --line-number -P '\bassertTrue\s*[\(;]' "$@" || grep_rc=$?
11+
case "$grep_rc" in
12+
0)
13+
echo "assertTrue() is not allowed! use assert() instead."; rc=1 ;;
14+
1)
15+
: ;; # code is good, do nothing
16+
*)
17+
echo "grep failed!"; rc=1 ;;
18+
esac
19+
exit "$rc"

‎test/functional/AccountDeletionRequestTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ private function assertNumberAccountDeletionRequests(int $x)
1111
$this->assertFalse($USER->hasRequestedAccountDeletion());
1212
$this->assertFalse($SQL->accDeletionRequestExists($USER->uid));
1313
} elseif ($x > 0) {
14-
$this->assertTrue($USER->hasRequestedAccountDeletion());
15-
$this->assertTrue($SQL->accDeletionRequestExists($USER->uid));
14+
assert($USER->hasRequestedAccountDeletion());
15+
assert($SQL->accDeletionRequestExists($USER->uid));
1616
} else {
1717
throw new RuntimeException("x must not be negative");
1818
}

‎test/functional/InvalidEPPNTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ public static function provider()
1515
public function testInitGetSSO(string $eppn, bool $is_valid): void
1616
{
1717
global $SSO;
18+
self::expectNotToPerformAssertions();
1819
$original_server = $_SERVER;
1920
$original_sso = $SSO;
2021
if (session_status() == PHP_SESSION_ACTIVE) {
@@ -37,6 +38,5 @@ public function testInitGetSSO(string $eppn, bool $is_valid): void
3738
$_SERVER = $original_server;
3839
$SSO = $original_sso;
3940
}
40-
$this->assertTrue(true); // if $is_valid, there are no other assertions
4141
}
4242
}

‎test/functional/NewUserTest.php‎

Lines changed: 39 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -164,11 +164,11 @@ public function testCreateUserByJoinGoupByPI($user_to_create_args, $expected_uid
164164
$pi_group = $USER->getPIGroup();
165165
$gid = $pi_group->gid;
166166
switchUser(...$user_to_create_args);
167-
$this->assertTrue(!$USER->exists());
167+
assert(!$USER->exists());
168168
$newOrg = new UnityOrg($SSO["org"], $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK);
169-
$this->assertTrue(!$newOrg->exists());
170-
$this->assertTrue($pi_group->exists());
171-
$this->assertTrue(!$pi_group->memberExists($USER));
169+
assert(!$newOrg->exists());
170+
assert($pi_group->exists());
171+
assert(!$pi_group->memberExists($USER));
172172
$this->assertRequestedMembership(false, $gid);
173173
try {
174174
$this->requestGroupMembership($pi_group->gid);
@@ -180,14 +180,14 @@ public function testCreateUserByJoinGoupByPI($user_to_create_args, $expected_uid
180180
// } catch(Exception) {
181181
// $second_request_failed = true;
182182
// }
183-
// $this->assertTrue($second_request_failed);
183+
// assert($second_request_failed);
184184
$this->assertRequestedMembership(true, $gid);
185185

186186
$this->cancelAllRequests();
187187
$this->assertRequestedMembership(false, $gid);
188188

189189
$this->requestGroupMembership($pi_group->gid);
190-
$this->assertTrue($pi_group->requestExists($USER));
190+
assert($pi_group->requestExists($USER));
191191
$this->assertRequestedMembership(true, $gid);
192192

193193
$REDIS->flushAll(); // regression test: flush used to break requests
@@ -197,11 +197,11 @@ public function testCreateUserByJoinGoupByPI($user_to_create_args, $expected_uid
197197
$this->approveUserByPI($approve_uid);
198198
switchUser(...$user_to_create_args);
199199

200-
$this->assertTrue(!$pi_group->requestExists($USER));
200+
assert(!$pi_group->requestExists($USER));
201201
$this->assertRequestedMembership(false, $gid);
202-
$this->assertTrue($pi_group->memberExists($USER));
203-
$this->assertTrue($USER->exists());
204-
$this->assertTrue($newOrg->exists());
202+
assert($pi_group->memberExists($USER));
203+
assert($USER->exists());
204+
assert($newOrg->exists());
205205

206206
$user_entry = $LDAP->getUserEntry($approve_uid);
207207
$qualified_user_group_entry = $LDAP->getGroupEntry($approve_uid);
@@ -217,9 +217,9 @@ public function testCreateUserByJoinGoupByPI($user_to_create_args, $expected_uid
217217
// } catch(Exception) {
218218
// $third_request_failed = true;
219219
// }
220-
// $this->assertTrue($third_request_failed);
220+
// assert($third_request_failed);
221221
$this->assertRequestedMembership(false, $gid);
222-
$this->assertTrue(!$pi_group->requestExists($USER));
222+
assert(!$pi_group->requestExists($USER));
223223
} finally {
224224
switchUser(...$user_to_create_args);
225225
$this->ensureOrgGroupDoesNotExist();
@@ -235,25 +235,25 @@ public function testCreateMultipleUsersByJoinGoupByPI()
235235
switchUser(...$pi_user_args);
236236
$pi_group = $USER->getPIGroup();
237237
$gid = $pi_group->gid;
238-
$this->assertTrue($pi_group->exists());
238+
assert($pi_group->exists());
239239
$users_to_create_args = getNonexistentUsersWithExistentOrg();
240240
try {
241241
foreach ($users_to_create_args as $user_to_create_args) {
242242
switchUser(...$user_to_create_args);
243-
$this->assertTrue(!$USER->exists());
244-
$this->assertTrue(!$pi_group->memberExists($USER));
243+
assert(!$USER->exists());
244+
assert(!$pi_group->memberExists($USER));
245245
$this->assertRequestedMembership(false, $gid);
246246
$this->requestGroupMembership($pi_group->gid);
247247
$this->assertRequestedMembership(true, $gid);
248248
$approve_uid = $USER->uid;
249249
switchUser(...$pi_user_args);
250-
// $this->assertTrue(!$pi_group->memberExists($USER));
250+
// assert(!$pi_group->memberExists($USER));
251251
$this->approveUserByPI($approve_uid);
252252
switchUser(...$user_to_create_args);
253-
$this->assertTrue(!$pi_group->requestExists($USER));
253+
assert(!$pi_group->requestExists($USER));
254254
$this->assertRequestedMembership(false, $gid);
255-
$this->assertTrue($pi_group->memberExists($USER));
256-
$this->assertTrue($USER->exists());
255+
assert($pi_group->memberExists($USER));
256+
assert($USER->exists());
257257
}
258258
} finally {
259259
foreach ($users_to_create_args as $user_to_create_args) {
@@ -272,11 +272,11 @@ public function testCreateUserByJoinGoupByAdmin($user_to_create_args, $expected_
272272
$pi_group = $USER->getPIGroup();
273273
$gid = $pi_group->gid;
274274
switchUser(...$user_to_create_args);
275-
$this->assertTrue(!$USER->exists());
275+
assert(!$USER->exists());
276276
$newOrg = new UnityOrg($SSO["org"], $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK);
277-
$this->assertTrue(!$newOrg->exists());
278-
$this->assertTrue($pi_group->exists());
279-
$this->assertTrue(!$pi_group->memberExists($USER));
277+
assert(!$newOrg->exists());
278+
assert($pi_group->exists());
279+
assert(!$pi_group->memberExists($USER));
280280
$this->assertRequestedMembership(false, $gid);
281281
try {
282282
$this->requestGroupMembership($pi_group->gid);
@@ -288,14 +288,14 @@ public function testCreateUserByJoinGoupByAdmin($user_to_create_args, $expected_
288288
// } catch(Exception) {
289289
// $second_request_failed = true;
290290
// }
291-
// $this->assertTrue($second_request_failed);
291+
// assert($second_request_failed);
292292
$this->assertRequestedMembership(true, $gid);
293293

294294
$this->cancelAllRequests();
295295
$this->assertRequestedMembership(false, $gid);
296296

297297
$this->requestGroupMembership($pi_group->getOwner()->getMail());
298-
$this->assertTrue($pi_group->requestExists($USER));
298+
assert($pi_group->requestExists($USER));
299299
$this->assertRequestedMembership(true, $gid);
300300

301301
$REDIS->flushAll(); // regression test: flush used to break requests
@@ -305,11 +305,11 @@ public function testCreateUserByJoinGoupByAdmin($user_to_create_args, $expected_
305305
$this->approveUserByAdmin($gid, $approve_uid);
306306
switchUser(...$user_to_create_args);
307307

308-
$this->assertTrue(!$pi_group->requestExists($USER));
308+
assert(!$pi_group->requestExists($USER));
309309
$this->assertRequestedMembership(false, $gid);
310-
$this->assertTrue($pi_group->memberExists($USER));
311-
$this->assertTrue($USER->exists());
312-
$this->assertTrue($newOrg->exists());
310+
assert($pi_group->memberExists($USER));
311+
assert($USER->exists());
312+
assert($newOrg->exists());
313313

314314
$user_entry = $LDAP->getUserEntry($approve_uid);
315315
$qualified_user_group_entry = $LDAP->getGroupEntry($approve_uid);
@@ -325,9 +325,9 @@ public function testCreateUserByJoinGoupByAdmin($user_to_create_args, $expected_
325325
// } catch(Exception) {
326326
// $third_request_failed = true;
327327
// }
328-
// $this->assertTrue($third_request_failed);
328+
// assert($third_request_failed);
329329
$this->assertRequestedMembership(false, $gid);
330-
$this->assertTrue(!$pi_group->requestExists($USER));
330+
assert(!$pi_group->requestExists($USER));
331331
} finally {
332332
switchUser(...$user_to_create_args);
333333
$this->ensureOrgGroupDoesNotExist();
@@ -342,10 +342,10 @@ public function testCreateUserByCreateGroup($user_to_create_args, $expected_uid_
342342
global $USER, $SSO, $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK;
343343
switchuser(...$user_to_create_args);
344344
$pi_group = $USER->getPIGroup();
345-
$this->assertTrue(!$USER->exists());
346-
$this->assertTrue(!$pi_group->exists());
345+
assert(!$USER->exists());
346+
assert(!$pi_group->exists());
347347
$newOrg = new UnityOrg($SSO["org"], $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK);
348-
$this->assertTrue(!$newOrg->exists());
348+
assert(!$newOrg->exists());
349349
try {
350350
$this->requestGroupCreation();
351351
$this->assertRequestedPIGroup(true);
@@ -356,7 +356,7 @@ public function testCreateUserByCreateGroup($user_to_create_args, $expected_uid_
356356
// } catch(Exception) {
357357
// $second_request_failed = true;
358358
// }
359-
// $this->assertTrue($second_request_failed);
359+
// assert($second_request_failed);
360360
$this->assertRequestedPIGroup(true);
361361

362362
$this->cancelAllRequests();
@@ -373,9 +373,9 @@ public function testCreateUserByCreateGroup($user_to_create_args, $expected_uid_
373373
switchUser(...$user_to_create_args);
374374

375375
$this->assertRequestedPIGroup(false);
376-
$this->assertTrue($pi_group->exists());
377-
$this->assertTrue($USER->exists());
378-
$this->assertTrue($newOrg->exists());
376+
assert($pi_group->exists());
377+
assert($USER->exists());
378+
assert($newOrg->exists());
379379

380380
$user_entry = $LDAP->getUserEntry($approve_uid);
381381
$qualified_user_group_entry = $LDAP->getGroupEntry($approve_uid);
@@ -391,7 +391,7 @@ public function testCreateUserByCreateGroup($user_to_create_args, $expected_uid_
391391
// } catch(Exception) {
392392
// $third_request_failed = true;
393393
// }
394-
// $this->assertTrue($third_request_failed);
394+
// assert($third_request_failed);
395395
$this->assertRequestedPIGroup(false);
396396
} finally {
397397
switchUser(...$user_to_create_args);

‎test/functional/PIMemberRequestTest.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,9 @@ public function testRequestMembership()
2929
$pi = $USER;
3030
$pi_group = $USER->getPIGroup();
3131
$gid = $pi_group->gid;
32-
$this->assertTrue($USER->isPI());
33-
$this->assertTrue($pi_group->exists());
34-
$this->assertTrue(arraysAreEqualUnOrdered([$pi], $pi_group->getGroupMembers()));
32+
assert($USER->isPI());
33+
assert($pi_group->exists());
34+
assert(arraysAreEqualUnOrdered([$pi], $pi_group->getGroupMembers()));
3535
$this->assertEquals([], $SQL->getRequests($gid));
3636
switchUser(...getUserNotPiNotRequestedBecomePi());
3737
$uid = $USER->uid;
@@ -40,13 +40,13 @@ public function testRequestMembership()
4040
$this->assertFalse($pi_group->memberExists($USER));
4141
try {
4242
$this->requestMembership($gid);
43-
$this->assertTrue($SQL->requestExists($uid, $gid));
43+
assert($SQL->requestExists($uid, $gid));
4444
$this->cancelRequest($gid);
4545
$this->assertFalse($SQL->requestExists($uid, $gid));
4646
$this->requestMembership("asdlkjasldkj");
4747
$this->assertContains("This PI doesn't exist", $_SESSION["MODAL_ERRORS"]);
4848
$this->requestMembership($pi_group->getOwner()->getMail());
49-
$this->assertTrue($SQL->requestExists($uid, $gid));
49+
assert($SQL->requestExists($uid, $gid));
5050
} finally {
5151
if ($SQL->requestExists($uid, $gid)) {
5252
$SQL->removeRequest($uid, $gid);

‎test/functional/PageLoadTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@ public static function provider()
2828
public function testLoadPage($user, $path)
2929
{
3030
global $USER, $SSO, $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK;
31+
self::expectNotToPerformAssertions();
3132
switchuser(...$user);
3233
http_get($path);
33-
$this->assertTrue(true); // assert there were no errors
3434
}
3535
}

‎test/functional/PiBecomeRequestTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ private function assertNumberPiBecomeRequests(int $x)
1111
if ($x == 0) {
1212
$this->assertFalse($SQL->requestExists($USER->uid));
1313
} elseif ($x > 0) {
14-
$this->assertTrue($SQL->requestExists($USER->uid));
14+
assert($SQL->requestExists($USER->uid));
1515
} else {
1616
throw new RuntimeException("x must not be negative");
1717
}
@@ -68,7 +68,7 @@ public function testRequestBecomePiUserRequestedAccountDeletion()
6868
switchUser(...getUserNotPiNotRequestedBecomePiRequestedAccountDeletion());
6969
$this->assertFalse($USER->isPI());
7070
$this->assertNumberPiBecomeRequests(0);
71-
$this->assertTrue($SQL->accDeletionRequestExists($USER->uid));
71+
assert($SQL->accDeletionRequestExists($USER->uid));
7272
try {
7373
http_post(__DIR__ . "/../../webroot/panel/account.php", [
7474
"form_type" => "pi_request",

‎test/functional/PiMemberApproveTest.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ private function requestJoinPI(string $gid)
4040

4141
private function assertGroupMembers(UnityGroup $group, array $members)
4242
{
43-
$this->assertTrue(arraysAreEqualUnOrdered($members, $group->getGroupMemberUIDs()));
43+
assert(arraysAreEqualUnOrdered($members, $group->getGroupMemberUIDs()));
4444
}
4545

4646
public function testApproveRequest()
@@ -55,7 +55,7 @@ public function testApproveRequest()
5555
$piUID = $USER->uid;
5656
$piGroup = $USER->getPIGroup();
5757

58-
$this->assertTrue($piGroup->exists());
58+
assert($piGroup->exists());
5959
$this->assertGroupMembers($piGroup, [$piUID]);
6060
$this->assertEmpty($piGroup->getRequests());
6161
try {
@@ -65,10 +65,10 @@ public function testApproveRequest()
6565

6666
switchUser(...$piSwitchArgs);
6767
$this->approveUser($uid);
68-
$this->assertTrue(!$piGroup->requestExists($user));
68+
assert(!$piGroup->requestExists($user));
6969
$this->assertEmpty($piGroup->getRequests());
7070
$this->assertGroupMembers($piGroup, [$piUID, $uid]);
71-
$this->assertTrue($piGroup->memberExists($user));
71+
assert($piGroup->memberExists($user));
7272
} finally {
7373
if ($piGroup->memberExists($user)) {
7474
$piGroup->removeUser($user);
@@ -89,7 +89,7 @@ public function testApproveNonexistentRequest()
8989
$piUID = $USER->uid;
9090
$piGroup = $USER->getPIGroup();
9191

92-
$this->assertTrue($piGroup->exists());
92+
assert($piGroup->exists());
9393
$this->assertGroupMembers($piGroup, [$piUID]);
9494
$this->assertEmpty($piGroup->getRequests());
9595
$this->assertFalse($piGroup->memberExists($user));

0 commit comments

Comments
 (0)