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.

Sign up for GitHub

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

Jump to bottom

Add PHP 7.2 testing #1811

Merged
merged 6 commits into from
May 1, 2017
Merged

Add PHP 7.2 testing #1811

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a74d771
Update travis to test against php 7.2
Apr 14, 2017
6570c1f
Add tests for countables
Apr 14, 2017
ed7db0f
Fix tcp proxy URL
Apr 14, 2017
9204658
Fix countable
Apr 14, 2017
f9eef35
Fix self:: to $this for test
scaytrase Apr 28, 2017
abdfd64
Add note on trailing slash fix
scaytrase May 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ php:
- 5.6
- 7.0
- 7.1
- nightly
- hhvm

before_script:
Expand All @@ -21,6 +22,7 @@ script: make test
matrix:
allow_failures:
- php: hhvm
- php: nightly
fast_finish: true

before_deploy:
Expand Down
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
},
"require-dev": {
"ext-curl": "*",
"phpunit/phpunit": "^4.0",
"phpunit/phpunit": "^4.0 || ^5.0",
"psr/log": "^1.0"
},
"autoload": {
Expand Down
2 changes: 1 addition & 1 deletion src/Handler/MockHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
*/
class MockHandler implements \Countable
{
private $queue;
private $queue = [];
private $lastRequest;
private $lastOptions;
private $onFulfilled;
Expand Down
5 changes: 5 additions & 0 deletions tests/Cookie/CookieJarTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ public function testCreatesFromArray()
$this->assertCount(2, $jar);
}

public function testEmptyJarIsCountable()
{
self::assertCount(0, new CookieJar());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why self:: not $this->. I think code should be uniform.

Copy link
Contributor

Choose a reason for hiding this comment

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

May be $this->assertInstanceOf will be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accepted $this->, will fix this.

Did not understand about instanceOf. This test explicitly invokes the count() method to be sure, that internals of CookieJar is countable from construction with 7.2

Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand, you want to check the existence of count method. Am I right? But actually you check that count == 0. So I'm suggesting you to check implementing of the \Countable interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I wan't to actually invoke the count method. Because PHP 7.2 throws a warning when trying to count a non-countable things like null, which could be done internally (i.e counting internal property which was not initialized) . So I want to check that CookieJar does not throws the warning when calling count directly after constructing.

https://wiki.php.net/rfc/counting_non_countables

The same thing I did with the MockHandler, but we have to fix it (see array default)

Copy link
Contributor

@Aliance Aliance Apr 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You wrote:

So I want to check that CookieJar does not throws the warning when calling count

But actually you

you check that count == 0

So nothing changes in my mind if you will check implementing of the \Countable interface, because it guarantees that calling count won't fail.

But it's kind of a holywar... =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having count method implemented does not guarantees that id does not fail. Short example:

class foo implements \Countable {
   private $things;

   public function count() { return count($this->things); }

   public function push($element) { $this->things[] = $element; } 
}

$f = new \foo();
$f->count(); // this will pass on PHP < 7.2 and throw warning on PHP 7.2
$f->push(1);
$f->count(); // this will pass

So having count method implemented does not guarantee that it works properly internally.

Checking that count == 0 is needed because the future phpunit versions will mark tests without assertions as risky, actually I could just call the (new CookieJar())->count() without any assertions.

}

public function testGetsCookiesByName()
{
$cookies = $this->getTestCookies();
Expand Down
5 changes: 5 additions & 0 deletions tests/Handler/MockHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ public function testIsCountable()
$this->assertCount(2, $mock);
}

public function testEmptyHandlerIsCountable()
{
$this->assertCount(0, new MockHandler());
}

/**
* @expectedException \InvalidArgumentException
*/
Expand Down
1 change: 1 addition & 0 deletions tests/Handler/StreamHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ public function testAddsProxy()
public function testAddsProxyByProtocol()
{
$url = str_replace('http', 'tcp', Server::$url);
$url = rtrim($url, '/');
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a quick not why we are doing this? Just to make sure we find it. Thanks

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, I've referred your #1823 issue

$res = $this->getSendResult(['proxy' => ['http' => $url]]);
$opts = stream_context_get_options($res->getBody()->detach());
$this->assertEquals($url, $opts['http']['proxy']);
Expand Down