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

tests/events/*.php - Allow optional runtime enforcement #28723

Merged
merged 6 commits into from
Dec 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 41 additions & 27 deletions Civi/Test/EventCheck.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,11 @@

namespace Civi\Test;

use PHPUnit\Framework\Assert;

/**
* An EventCheck is a fragment of a unit-test -- it is mixed into
* various test-scenarios and applies extra assertions.
*/
class EventCheck extends Assert {

/**
* @var \PHPUnit\Framework\Test
*/
private $test;
class EventCheck {

/**
* Determine whether this check should be used during the current test.
Expand All @@ -38,20 +31,6 @@ public function isSupported($test) {
return TRUE;
}

/**
* @return \PHPUnit\Framework\Test|NULL
*/
public function getTest() {
return $this->test;
}

/**
* @param \PHPUnit\Framework\Test|null $test
*/
public function setTest($test): void {
$this->test = $test;
}

/**
* Assert that a variable has a given type.
*
Expand All @@ -61,21 +40,56 @@ public function setTest($test): void {
* Ex: [`array`, `NULL`, `CRM_Core_DAO`]
* @param mixed $value
* The variable to check
* @param string|null $msg
* @param string|null $message
* @see \CRM_Utils_Type::validatePhpType
*/
public function assertType($types, $value, ?string $msg = NULL) {
public function assertType($types, $value, string $message = '') {
if (!\CRM_Utils_Type::validatePhpType($value, $types, FALSE)) {
$defactoType = is_object($value) ? get_class($value) : gettype($value);
$types = is_array($types) ? implode('|', $types) : $types;
$this->fail(sprintf("Expected one of (%s) but found %s\n%s", $types, $defactoType, $msg));
$comment = sprintf('Expected one of (%s) but found %s', $types, $defactoType);
static::fail(trim("$message\n$comment"));
}
}

public function setUp() {
public static function assertEquals($expected, $actual, string $message = ''): void {
if ($expected !== $actual) {
$comment = sprintf('Expected value %s and actual value %s should match.', json_encode($expected), json_encode($actual));
static::fail(trim("$message\n$comment"));
}
}

public static function assertContains($needle, iterable $haystack, string $message = ''): void {
$haystack = ($haystack instanceof \Traversable) ? iterator_to_array($haystack) : (array) $haystack;
if (!in_array($needle, $haystack)) {
$comment = sprintf('Item %s not found in list: %s', json_encode($needle), json_encode($haystack));
static::fail(trim("$message\n$comment"));
}
}

public static function assertMatchesRegularExpression(string $pattern, string $string, string $message = ''): void {
if (!preg_match($pattern, $string)) {
$comment = sprintf('Value %s does not match pattern %s.', json_encode($string), json_encode($pattern));
static::fail(trim("$message\n$comment"));
}
}

public static function assertNotEmpty($actual, string $message = ''): void {
if (empty($actual)) {
$comment = 'Value should not be empty.';
static::fail(trim("$message\n$comment"));
}
}

public static function assertTrue($condition, string $message = ''): void {
if ($condition !== TRUE) {
$comment = 'Value should be TRUE.';
static::fail(trim("$message\n$comment"));
}
Comment on lines +77 to +88
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing these are just what we need right now and no more, but it feels weird to have assertNotEmpty and not assertEmpty; assertTrue and not assertFalse.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@artfulrobot Yup, exactly. I'm sure the vocabulary of assertions could be tuned-up more going forward.

}

public function tearDown() {
public static function fail(string $message = ''): void {
throw new EventCheckException($message);
}

}
24 changes: 24 additions & 0 deletions Civi/Test/EventCheckException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

namespace Civi\Test;

/**
* The "event checks" ensure that hooks/events satisfy some expected contracts.
* This exception is emitted if there is a non-conformant hook/event.
* It should only be emitted on development environments.
*/

if (class_exists('PHPUnit\Framework\AssertionFailedError')) {

class EventCheckException extends \PHPUnit\Framework\AssertionFailedError {

}

}
else {

class EventCheckException extends \RuntimeException {

}

}
9 changes: 1 addition & 8 deletions Civi/Test/EventChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class EventChecker {
private $activeChecks = NULL;

/**
* @param \PHPUnit\Framework\Test $test
* @param \PHPUnit\Framework\Test|NULL $test
*
* @return $this
*/
Expand All @@ -37,9 +37,7 @@ public function start($test) {
/** @var EventCheck $template */
if ($template->isSupported($test)) {
$checker = clone $template;
$checker->setTest($test);
$this->activeChecks[] = $checker;
$checker->setUp();
}
}
}
Expand Down Expand Up @@ -76,11 +74,6 @@ public function addListeners() {
*/
public function stop() {
// NOTE: In test environment, dispatcher will be removed regardless.
foreach ($this->activeChecks ?? [] as $checker) {
/** @var \Civi\Test\EventCheck $checker */
Invasive::call([$checker, 'tearDown']);
$checker->setTest(NULL);
}
$this->activeChecks = NULL;
return $this;
}
Expand Down
16 changes: 15 additions & 1 deletion tests/events/hook_civicrm_links.evch.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

return new class() extends EventCheck implements HookInterface {

// There are several properties named "$grandfatherdXyz". These mark itmes which
// There are several properties named "$grandfatherdXyz". These mark items which
// pass through hook_civicrm_links but deviate from the plain interpretation of the docs.
// Auditing or cleaning each would be its own separate project. Please feel free to
// do that audit and figure how to normalize it. But for now, the goal of this file is
Expand Down Expand Up @@ -69,6 +69,16 @@
'pcp.user.actions::Pcp',
];

/**
* List of events with multiple problems. These are completely ignored.
*
* @var string[]
*/
protected $unrepentantMiscreants = [
'create.new.shortcuts', /* FIXME */
'create.new.shorcuts', /* Deprecated */
];

/**
* Ensure that the hook data is always well-formed.
*
Expand All @@ -78,6 +88,10 @@ public function hook_civicrm_links($op, $objectName, &$objectId, &$links, &$mask
// fprintf(STDERR, "CHECK hook_civicrm_links($op)\n");
$msg = sprintf('Non-conforming hook_civicrm_links(%s, %s)', json_encode($op), json_encode($objectName));

if (in_array($op, $this->unrepentantMiscreants)) {
return;
}

$this->assertTrue((bool) preg_match(';^\w+(\.\w+)+$;', $op), "$msg: Operation ($op) should be dotted expression");
$this->assertTrue((bool) preg_match(';^[A-Z][a-zA-Z0-9]+$;', $objectName) || in_array($objectName, $this->grandfatheredObjectNames),
"$msg: Object name ($objectName) should be a CamelCase name or a grandfathered name");
Expand Down
Loading