From cc7ed47d9f0b515d7ec829a0dd8bb4d321c53df5 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Thu, 23 Feb 2023 13:52:10 +1300 Subject: [PATCH] Reduce processing load in test assertAPIFailure In trying to figure out why adding 'too much' (an exception) to the return caused a memory out I realised that the print_r here is realised regardless of whether it is needed. This fixes to only resolve if it is going to be displayed. Arguably this would have been a case where an inline function would have made sense - but at the cost of readability - ie it's more helpful to add code comments to explain the if than to make the codd hard to follow. The only downside is the hypothetical possibility of is_error being something other than 1,'1',TRUE - I think if we thought that was a remote possibilty we would add a unit test to add it - not cover it in an assertion --- Civi/Test/Api3TestTrait.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Civi/Test/Api3TestTrait.php b/Civi/Test/Api3TestTrait.php index 22e3058cfd..49d81e97b4 100644 --- a/Civi/Test/Api3TestTrait.php +++ b/Civi/Test/Api3TestTrait.php @@ -74,16 +74,20 @@ trait Api3TestTrait { * Api result. * @param string $prefix * Extra test to add to message. - * @param null $expectedError + * @param string|null $expectedError */ - public function assertAPIFailure($apiResult, $prefix = '', $expectedError = NULL) { + public function assertAPIFailure(array $apiResult, string $prefix = '', ?string $expectedError = NULL): void { if (!empty($prefix)) { $prefix .= ': '; } if ($expectedError && !empty($apiResult['is_error'])) { $this->assertStringContainsString($expectedError, $apiResult['error_message'], 'api error message not as expected' . $prefix); } - $this->assertEquals(1, $apiResult['is_error'], "api call should have failed but it succeeded " . $prefix . (print_r($apiResult, TRUE))); + if (!$apiResult['is_error']) { + // This section only called when it is going to fail - that means we don't have to parse the print_r in the message + // if it is not going to be used anyway. It's really helpful for debugging when needed, but potentially expensive otherwise. + $this->fail('api call should have failed but it succeeded ' . $prefix . (print_r($apiResult, TRUE))); + } $this->assertNotEmpty($apiResult['error_message']); } -- 2.25.1