Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Added unit tests for the Metadata class #4

Closed
wants to merge 2 commits into from
Closed

Added unit tests for the Metadata class #4

wants to merge 2 commits into from

Conversation

robertmarsal
Copy link
Contributor

This pull requests adds unit tests, with 100% coverage, for the Metadata class.

@@ -41,6 +41,7 @@ public function __construct(Adapter $adapter)
*
* @param Adapter $adapter
* @return Source\AbstractSource
* @throws \Exception
Copy link
Member

Choose a reason for hiding this comment

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

Add description when this exception is thrown

@robertmarsal
Copy link
Contributor Author

@Maks3w I have update the pull request with your feedback from the code review, thanks!

->disableOriginalConstructor()
->getMock();

$adapterMock->expects($this->once())
Copy link
Member

Choose a reason for hiding this comment

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

Could you extract all the thing about adapterMock in a method called createAdapterMockWithPlatform($platform)

I think will improve test readiblity.

@Maks3w
Copy link
Member

Maks3w commented Jun 6, 2015

I start to think this class should be deprecated and only to have a factory class with a method for get the source from adapter

@robertmarsal
Copy link
Contributor Author

I am not actually using the class. I just saw it had no coverage and I had some spare time so I decided to write tests for it. It does look like the only useful bit is the switch to get the source instance, as all the rest of the methods just proxy to the source.

I will leave the pull request as is for now, until a decision is made

@Maks3w
Copy link
Member

Maks3w commented Jun 6, 2015

All the thing about to test proxy methods reveal a bad design flow.

@mwillbanks
Copy link
Contributor

@Maks3w that seems fair; however, i don't think it a bad idea to add the unit tests that have been add here. Actually it is very well likely a great idea for the time being. I would say we should merge the PR and then we can deprecate the other part in the future.

@Maks3w
Copy link
Member

Maks3w commented Jul 17, 2015

I think I wrote a PoC with a more clean structure let me to dig on it

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

Successfully merging this pull request may close these issues.

None yet

3 participants