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

[4.0] PHPUnit test fixes #14084

Merged
merged 3 commits into from Feb 17, 2017
Merged

[4.0] PHPUnit test fixes #14084

merged 3 commits into from Feb 17, 2017

Conversation

yvesh
Copy link
Member

@yvesh yvesh commented Feb 15, 2017

Summary of Changes

This should fix the unit tests in the 4.0 branch

Testing Instructions

Review & Travis

Expected result

Unit Tests should pass

Documentation Changes Required

None

@zero-24
Copy link
Member

zero-24 commented Feb 15, 2017

There was 1 error:
1) JDocumentFeedTest::testEnsureAddItemReturnsThisObject
Exception: DateTimeZone::__construct(): Unknown or bad timezone ()

/home/travis/build/joomla/joomla-cms/libraries/joomla/document/feed.php:179
/home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/joomla/document/feed/JDocumentFeedTest.php:32

@@ -38,6 +38,6 @@ public function testConstructorSetsStreamTransport()
{
$http = new JHttp;

$this->assertAttributeInstanceOf('JHttpTransportStream', 'transport', $http);
$this->assertAttributeInstanceOf('JHttpTransportCurl', 'transport', $http);
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't quite right. The JMediawikiHttp constructor is telling the JHttpFactory it should only allow a JHttpTransportStream object to be created, so why it's returning a JHttpTransportCurl instance needs to be investigated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Look at the line above in the factory, it's supposed to be setting the default value to an array so it should be supporting both strings and arrays in theory.

Copy link
Member Author

@yvesh yvesh Feb 15, 2017

Choose a reason for hiding this comment

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

@mbabker Look some lines below: https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/joomla/http/factory.php#L77

That can't work with a string (foreach)

if (is_string($availableAdapters))
{
     $availableAdapters = array($availableAdapters);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/joomla/http/factory.php#L67 should be setting $default as an array before assigning it to $availableAdapters so if that's not happening we've got another bug on our hands.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, overread that line. Going to debug it tomorrow morning..

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually when i look at this test again, the whole test makes no sense.. as we are creating a new JHttp instance and not a JMediawikiHttp.. And JHttp without parameters, which takes the first available adapter.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is why I shouldn't read pull requests before ☕️.

@zero-24 zero-24 added this to the Joomla 4.0 milestone Feb 16, 2017
@yvesh
Copy link
Member Author

yvesh commented Feb 17, 2017

@mbabker Travis passes now, can you review?

@zero-24 zero-24 merged commit 6445be4 into joomla:4.0-dev Feb 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants