Skip to content

Add xcgmock #2595

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

Merged
merged 8 commits into from
Mar 21, 2019
Merged

Add xcgmock #2595

merged 8 commits into from
Mar 21, 2019

Conversation

wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Mar 20, 2019

This defines a means of using gmock matchers in XCTest tests.

This makes it easy to fix tests currently depending on e.g. NSArray values and XCTAssertEqualObjects to instead use something else. It's better to fix the test to

XC_ASSERT_THAT(accum, ElementsAre(viewSnapshot1));

Instead of:

XCTAssertEqual(accum, (std::vector<ViewSnapshot>(viewSnapshot1)));

... or the even worse:

XCTAssertTrue(accum == std::vector<ViewSnapshot>(viewSnapshot1));

Copy link
Contributor

@var-const var-const left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks for the PR! I wanted something like this for a long time.


#define XC_ASSERT_THAT(actual, matcher) \
do { \
using actual_type = \
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this alias appears unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Yes. This was used previously when I was casting the matchers by hand before I figured out that I could use PredicateFormatterFromMatcher.

I'm still somewhat uncomfortable about using testing::internal here, but I think it's worth accepting that we might be broken on the next upgrade to keep this simple.

inline void ObjcPrintTo(id value, std::ostream* os) {
// Force the result type to NSString* or we can't resolve MakeString.
NSString* description = [value description];
*os << util::MakeString(description);
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 100% sure util::ToString (from to_string.h) works with id parameters, but if it does, perhaps use it here?

*os << util::ToString(value);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea. It would allow me to get rid of ObjcPrintTo too.

Unfortunately, util::ToString does not work on types that are only forward declarations, as are most of the types below. This version works because the type is cast to id and thus relies on fully dynamic dispatch.

TEST(XcGmockTest, FoundationPrints) {
EXPECT_EQ("value", testing::PrintToString(@"value"));

std::string expected = util::MakeString([@[ @"value" ] description]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Very optional: I think this could be

std::string expected = util::ToString(@[@"value"]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@var-const
Copy link
Contributor

var-const commented Mar 20, 2019

Note: I don't think

XCTAssertEqual(accum, std::vector<ViewSnapshot>(viewSnapshot1));

works -- at least, I have never been able to get it to work, because something in the macro checks that the types involved are primitives. I had to do

XCTAssertTrue(accum == std::vector<ViewSnapshot>(viewSnapshot1));

which is even worse, because it doesn't give a good message on failure.

@wilhuff
Copy link
Contributor Author

wilhuff commented Mar 20, 2019

Note: I don't think ...

Right, it actually takes another set of parens to force the C preprocessor to be able to understand that it's actually a single argument.

XCTAssertEqual(accum, (std::vector<ViewSnapshot>(viewSnapshot1)));

I've updated the description, but this way's better anyway.

@@ -100,6 +100,7 @@ class ABSL_MUST_USE_RESULT Status {
/// \brief Return a string representation of this status suitable for
/// printing. Returns the string `"OK"` for success.
std::string ToString() const;
friend std::ostream& operator<<(std::ostream& out, const Status& status);
Copy link
Member

Choose a reason for hiding this comment

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

The definition doesn't actually use any private fields. I think you can just drop this friend declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My goal is to declare these near the ToString() to make it obvious which classes support this usage and which don't yet. I want to make this boilerplate something you can mostly copypasta into the next class.

In addition to giving access to internals friend declares a free function but inside the body of the class. There's no equivalent way to declare this operator without moving it outside the class, which is annoying.

See: https://stackoverflow.com/a/2315264/6253640

@@ -177,6 +178,7 @@ class ViewSnapshot {
}

std::string ToString() const;
friend std::ostream& operator<<(std::ostream& out, const ViewSnapshot& value);
Copy link
Member

Choose a reason for hiding this comment

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

I think you can drop the friend declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropping friend also means moving the declaration out of the class, which is annoying.

@@ -148,6 +149,8 @@ class Timestamp {
* don't rely on the format of the string.
*/
std::string ToString() const;
friend std::ostream& operator<<(std::ostream& out,
Copy link
Member

Choose a reason for hiding this comment

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

(again)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope.

@rsgowman rsgowman removed their assignment Mar 20, 2019
@wilhuff wilhuff merged commit 38815b0 into master Mar 21, 2019
@wilhuff wilhuff deleted the wilhuff/xcgmock branch March 21, 2019 21:48
Corrob pushed a commit that referenced this pull request Apr 25, 2019
* Add operator<< to a few types

* Add xcgmock

* Use gmock matchers in XCTest

* Add gmock to integration test header search paths
@firebase firebase locked and limited conversation to collaborators Oct 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants