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.

Sign up for GitHub

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

Jump to bottom

chore: Extract compareResult test helper method to Core #6828

Closed
wants to merge 1 commit into from
Closed

chore: Extract compareResult test helper method to Core #6828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 21 additions & 0 deletions Core/src/Testing/TestHelpers.php
View file
Open in desktop
Expand Up @@ -24,6 +24,7 @@
use Google\Cloud\Core\Testing\Snippet\Coverage\Scanner;
use Google\Cloud\Core\Testing\Snippet\Parser\Parser;
use Google\Cloud\Core\Testing\System\SystemTestCase;
use PHPUnit\Framework\Assert;

/**
* Class TestHelpers is used to hold static functions required for testing
Expand Down Expand Up @@ -256,6 +257,26 @@ public static function getPrivateProperty($class, $property)
return $c($class);
}

/**
* Compare actual and expected results when the requirement for comparision
* is `===` including outlier cases like:
* - Comparing NAN
* - Comparing floating point values with some delta
*/
public static function compareResult($expected, $actual)
{
if (is_float($expected)) {
if (is_nan($expected)) {
Assert::assertNan($actual);
} else {
Assert::assertEqualsWithDelta($expected, $actual, 0.01);
}
} else {
// Used because assertEquals(null, '') doesn't fails
Assert::assertSame($expected, $actual);
}
}
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 not a huge fan of static calls. I think we would be better off putting this in Google\Cloud\Core\Testing\System\SystemTestCase, which is extended by Firestore and Datastore tests? This way we could override the function, and also not have to use static calls to TestHelpers. This has the additional benefit that we can also replace the static calls to Assert with standard TestCase functions:

$this->assertNan($actual);
$this->assertEqualsWithDelta($expected, $actual, 0.01);
$this->assertSame($expected, $actual);

Copy link
Contributor Author

@yash30201 yash30201 Nov 30, 2023 •
edited

Choose a reason for hiding this comment

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

We heavily need them for unit testing as they are separated in files much more than system tests.

And since Unit tests extends the PHPUnit's TestCase, our only options are:

  1. Go with static functions by implementing them in TestHelpers.
  2. Create a new Trait in Core/src/Testing and use it in required unit tests and SystemTestCase


/**
* Determine the path of the project root based on where the composer
* autoloader is located.
Expand Down
11 changes: 6 additions & 5 deletions Firestore/tests/System/AggregateQueryTest.php
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@

use Exception;
use Google\Cloud\Core\Exception\BadRequestException;
use Google\Cloud\Core\Testing\TestHelpers;
use Google\Cloud\Core\Timestamp;
use Google\Cloud\Firestore\Aggregate;
use Google\Cloud\Firestore\Filter;
Expand Down Expand Up @@ -111,7 +112,7 @@ public function testMultipleAggregationsOverDifferentFields($aggregates, $expect

$querySnapshot = $query->getSnapshot();
foreach ($expectedResults as $key => $value) {
$this->compareResult($value, $querySnapshot->get($key));
TestHelpers::compareResult($value, $querySnapshot->get($key));
}
}

Expand Down Expand Up @@ -144,7 +145,7 @@ private function assertQuery(Query $query, $type, $arg, $expected)

$actual = $arg ? $query->$type($arg) : $query->$type();

$this->compareResult($expected, $actual);
TestHelpers::compareResult($expected, $actual);
$this->assertQueryWithMultipleAggregations($query, $type, $arg, $expected);
}

Expand Down Expand Up @@ -174,7 +175,7 @@ private function assertQueryWithMultipleAggregations(Query $query, $type, $arg,

foreach ($expectedResults as $key => $expectedResult) {
$actualResult = $snapshot->get($key);
$this->compareResult($expected, $actualResult);
TestHelpers::compareResult($expected, $actualResult);
}

$this->assertEquals(0, strlen($snapshot->getTransaction()));
Expand Down Expand Up @@ -288,7 +289,7 @@ public function getWhereCases()
// For testing where: equality for random value
['count', null, '=', $randomVal, 1, [['value' => $randomVal]]],
['sum', 'value', '=', $randomInt, $randomInt, [['value' => $randomInt]]],
['avg', 'value', '=', $randomInt, $randomInt, [['value' => $randomInt]]],
['avg', 'value', '=', $randomInt, (float)$randomInt, [['value' => $randomInt]]],

// For testing where: equality for null
['count', null, '=', null, 1, [['value' => null]]],
Expand Down Expand Up @@ -341,7 +342,7 @@ public function getSnapshotCursorCases()
return [
['count', null, [4, 3, 3, 4], $docsToAdd],
['sum', 'value', [10, 9, 6, 10], $docsToAdd],
['avg', 'value', [2.5, 3, 2, 2.5], $docsToAdd]
['avg', 'value', [2.5, 3.0, 2.0, 2.5], $docsToAdd]
];
}

Expand Down