Skip to content
This repository has been archived by the owner on Jul 8, 2023. It is now read-only.

Default "empty" values for types #150

Closed
ezzatron opened this issue Apr 28, 2016 · 11 comments
Closed

Default "empty" values for types #150

ezzatron opened this issue Apr 28, 2016 · 11 comments
Milestone

Comments

@ezzatron
Copy link
Contributor

ezzatron commented Apr 28, 2016

Can I get some input on what would be the best options for default "empty" values for some built-in types? These currently drive Default values for return types, but in future may also drive some auto-wiring of type-hinted function parameters.

Here's what Phony currently handles:

Type Value
(none) null
bool false
int 0
float .0
string ''
array []
stdClass (object) []
callable function () {}
Traversable new EmptyIterator()
Iterator new EmptyIterator()
Generator (function () {return; yield;})()
<type> mock(<type>)->mock()

I'm thinking that the EmptyIterator ones are a strange outlier here, in that you can't wrap them in a mock handle and verify their interactions. Perhaps they should be partial mocks? There's also a few in-built classes not catered for.

Here's some changes I'm considering:

Type Value
Traversable partialMock('EmptyIterator')->mock()
Iterator partialMock('EmptyIterator')->mock()
Exception partialMock('Exception')->mock()
RuntimeException etc. partialMock('RuntimeException')->mock()
Error partialMock('Error')->mock()
AssertionError etc. partialMock('AssertionError')->mock()
callable spy()
Closure function () {}
@ezzatron ezzatron added this to the Next release milestone Apr 28, 2016
@ezzatron
Copy link
Contributor Author

@jmalloc @koden-km @darianbr @parkertr pls halp

@jmalloc
Copy link
Member

jmalloc commented Apr 28, 2016

Closure should be function () {} for sure.

I'm leaning towards the partial mock route. It offers consistency with the behaviour of user-defined types, while also provided a working object. I'm less convinced about the exceptions/errors though, could these just be full mocks like all other types?

I would add that making the 'empty' value for callable a spy would be more useful than the empty closure. Likewise, Generator could be a generator spy.

@jmalloc
Copy link
Member

jmalloc commented Apr 28, 2016

Further to that, is there any reason to treat stdClass specially? It could also just be a mock, I think.

@koden-km
Copy link

Is there a difference in making a stdClass like (object) [] vs new stdClass() ?

@jmalloc
Copy link
Member

jmalloc commented Apr 28, 2016

Is there a difference in making a stdClass like (object) [] vs new stdClass() ?

Nope.

I guess, regarding stdClass, there's possibly no compelling argument for a mock, since it has no methods (but mocks do have some property support, iirc?). That said, keeping the set of special cases as small as possible feels like a reasonable goal to include.

@ezzatron
Copy link
Contributor Author

I guess, regarding stdClass, there's possibly no compelling argument for a mock, since it has no methods (but mocks do have some property support, iirc?).

That was my logic with stdClass, yeah. No good reason to mock it. Phony's property support only really extends to ah-hoc mocks, which has no relevance here. It could also be detrimental if the value is to be cast to an array or JSON encoded, two common use cases for stdClass.

I'm less convinced about the exceptions/errors though, could these just be full mocks like all other types?

They could. It's just that, since they all have constructors that require no arguments, you get a fully working exception "for free" that already has an integer code, string message etc.

I would add that making the 'empty' value for callable a spy would be more useful than the empty closure.

Good point. Probably doesn't work for Closure unfortunately, but still worthwhile.

Likewise, Generator could be a generator spy.

I don't think this can be done. Unless you can think of a way to get the original spy out of the Generator object?

@jmalloc
Copy link
Member

jmalloc commented Apr 28, 2016

That was my logic with stdClass, yeah. No good reason to mock it.

Yep, agreed. JSON encoding, etc is a good point.

(re: partial mocks of throwables) ... you get a fully working exception "for free" that already has an integer code, string message etc.

Seems fair enough, so this would extend to all concrete, built-in throwable types? Is it worth bringing other built-in types back into the discussion (we dismissed the SPL collection types in an earlier discussion).

(re: generator spies) I don't think this can be done. Unless you can think of a way to get the original spy out of the Generator object?

I hadn't considered this (obviously). It is possible to assign arbitrary properties to Generator, so perhaps it could be stuffed in there as $generator->phonySpy. Rather than accessing this directly, it could be made available via the facade, e.g. Phony::getTheSpy($generator), adding a safety net if the arbitary-property method breaks in the future.

It's not pretty, but I do suspect getting a generator spy by default could be quite useful, especially in Recoil.

@ezzatron
Copy link
Contributor Author

Seems fair enough, so this would extend to all concrete, built-in throwable types? Is it worth bringing other built-in types back into the discussion (we dismissed the SPL collection types in an earlier discussion).

Yes, probably. Perhaps some kind of guideline like, if it's built in, can be created with an empty constructor, and has no side effects, then partial mock it?

It is possible to assign arbitrary properties to Generator, so perhaps it could be stuffed in there as $generator->phonySpy. Rather than accessing this directly, it could be made available via the facade, e.g. Phony::getTheSpy($generator), adding a safety net if the arbitary-property method breaks in the future.

Yeah, it's probably possible right now from a technical standpoint. Not sure if I want to introduce another facade method for it. Let me think some more about that one.

@ezzatron
Copy link
Contributor Author

After writing some more extensive tests for this stuff, it turns out that in the majority of cases, a partial mock is not necessary. By this I mean that:

  • Full mocks of iterators already behave as if empty, with no errors or warnings when iterated.
  • Full mocks of Countable, ArrayAccess, the SPL collections, etc. are similar (e.g. returning integer 0 for count($mock) in the case of Countable).
  • All throwables are similar, returning '' for $mock->getMessage(), and 0 for $mock->getCode().

There were a few other edge cases, and the process highlighted some inbuilt classes that Phony is having trouble mocking currently (#151).

@ezzatron
Copy link
Contributor Author

@jmalloc re generators, I realized that spying on generators won't be a problem here. This should work fine:

$spy = spy(function (): Generator {});
$generator = $spy();

$generator->send('x');
$spy->received('x'); // should pass

But perhaps you meant stubbing the returned generator? There would be no way to do that at the moment. I'm not sure it really makes sense anyway, I can't think of a good use-case.

@ezzatron
Copy link
Contributor Author

Okay, so that example is bad.

Firstly, you need a stub, not a spy. Secondly, the generator spies are working, but as it turns out, there's not much that can be verified on an empty generator anyway. It never receives anything because it doesn't yield, and of course it never produces anything, and we don't yet have a consumed() verification or anything similar. But this does work:

$stub = stub(eval('return function (): Generator {};'))->returns();
iterator_to_array($stub());

$stub->producedAll();

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants