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

chore: Extract compareResult test helper method to Core #6828

Closed

Conversation

yash30201
Copy link
Contributor

@yash30201 yash30201 commented Nov 29, 2023

With the refactoring of tests cleanly use dataproviders with multiple cases in the single testing method, we expect the test method to receive multitude of data types requiring same assertEqualts. Few edge cases arising in those situations are:

  • Asserting NANs
  • Asserting null is equal to ''
  • Asserting floats with some delta

This requirement is both is Firestore and Datastore (actually "will be" until #6817) merges. Therefore it's better to extract this functionality to Core/src/Testing.

@yash30201 yash30201 requested review from a team as code owners November 29, 2023 13:12
// 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

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

@yash30201 yash30201 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 1, 2023
@yash30201 yash30201 closed this May 8, 2024
@yash30201 yash30201 deleted the chore-extract-compare-result branch May 8, 2024 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants