[WIP] Testing and adapting for php7 if necessary. (Including an increase in Code Coverage) #47

WillSkates · 2015-12-16T11:19:51Z

No description provided.

garemoko · 2015-12-16T11:28:28Z

I'm assuming this PR is a work in progress. See #46 which may be helpful.

WillSkates · 2015-12-16T11:40:16Z

Yep you're correct thanks for the link. I've changed the title to reflect.

WillSkates · 2015-12-21T19:57:03Z

Ready for merge.

garemoko · 2015-12-28T11:38:22Z

@johnpbloch as you raised issue #46, would you like to review this PR?

WillSkates · 2016-01-11T10:56:28Z

@garemoko Are you happy with this PR?

garemoko · 2016-01-11T13:45:04Z

@WillSkates thanks for the poke. I was hoping that @johnpbloch might be set up to test this.

Nevertheless, I'll add this to me to-do list to test this branch with php 5.4 which I have installed now, and then see if I can upgrade to php 7 and test with that.

johnpbloch · 2016-01-11T14:52:52Z

Sorry, I was travelling at the time, and this fell off my radar since then. I'm checking the tests right now. Regarding a code review, I'm very much not familiar with the codebase, so I'll leave that to someone who is better qualified to review it.

johnpbloch · 2016-01-11T15:04:49Z

Well, the PHP7 tests aren't passing for me locally, but neither are the tests in master, so this is probably something wrong with my environment. I will try again later.

WillSkates · 2016-01-12T15:40:02Z

@johnpbloch No problem, thank you for coming back so quickly after the poke. Could you please paste your output? You will need to provide some configuration before you run them i.e. https://github.com/WillSkates/TinCanPHP/blob/master/tests/Config.php.travis-ci

@garemoko It might be worth looking at using a VM or a Docker image to run the test suite in.

johnpbloch · 2016-01-12T16:11:47Z

Output of tests on master:

TinCanPHP (master=) $ /c/PHP56/php vendor/phpunit/phpunit/phpunit tests
PHPUnit 5.1.4 by Sebastian Bergmann and contributors.

..............................................................  62 / 189 ( 32%)
.........................F.F............F...F................. 124 / 189 ( 65%)
.......................................E......FFFF............ 186 / 189 ( 98%)
...                                                            189 / 189 (100%)

Time: 11.73 seconds, Memory: 7.00Mb

There was 1 error:

1) StatementTest::testVerifyIncorrectPubKey
openssl_pkey_get_details() expects parameter 1 to be resource, boolean given

C:\Users\John\Projects\TinCanPHP\tests\StatementTest.php:811

--

There were 8 failures:

1) RemoteLRSTest::testSaveStatementsWithAttachments
success
Failed asserting that false is true.

C:\Users\John\Projects\TinCanPHP\tests\RemoteLRSTest.php:171

2) RemoteLRSTest::testRetrieveStatementWithAttachments
attachment content
Failed asserting that null is identical to '{"foo":"bar"}'.

C:\Users\John\Projects\TinCanPHP\tests\RemoteLRSTest.php:242

3) RemoteLRSTest::testRetrieveActivity
retrieved activity
'{"error":true,"success":false...ce"}]}' does not match expected type "object".

C:\Users\John\Projects\TinCanPHP\tests\RemoteLRSTest.php:481

4) RemoteLRSTest::testRetrievePerson
retrieved person
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 TinCan\Person Object (
     'objectType' => 'Person'
     'name' => Array (
-        0 => 'Example User'
     )
     'mbox' => Array (
-        0 => 'mailto:[email protected]'
     )
-    'mbox_sha1sum' => null
-    'openid' => null
-    'account' => null
+    'mbox_sha1sum' => Array ()
+    'openid' => Array ()
+    'account' => Array ()
 )

C:\Users\John\Projects\TinCanPHP\tests\RemoteLRSTest.php:541

5) StatementVariationsTest::testAttachmentsString
successful request
Failed asserting that false is true.

C:\Users\John\Projects\TinCanPHP\tests\StatementVariationsTest.php:116

6) StatementVariationsTest::testAttachmentsBinaryFile
Failed asserting that false is true.

C:\Users\John\Projects\TinCanPHP\tests\StatementVariationsTest.php:152

7) StatementVariationsTest::testSignedAndVerified
verify signature
Failed asserting that false is true.

C:\Users\John\Projects\TinCanPHP\tests\StatementVariationsTest.php:186

8) StatementVariationsTest::testSignedAndVerifiedX5c
verify signature
Failed asserting that false is true.

C:\Users\John\Projects\TinCanPHP\tests\StatementVariationsTest.php:220

FAILURES!
Tests: 189, Assertions: 711, Errors: 1, Failures: 8.


Non-exception error occurred:

2: openssl_x509_read(): supplied parameter cannot be coerced into an X509 certificate!
C:\Users\John\Projects\TinCanPHP\src\Statement.php (254)

Output from the php7 branch:

TinCanPHP (php7=) $ vendor/bin/phpunit tests
PHPUnit 5.1.4 by Sebastian Bergmann and contributors.

..............................................................  62 / 191 ( 32%)
...........................F.F............F...F............... 124 / 191 ( 64%)
.........................................E......FFFF.......... 186 / 191 ( 97%)
.....                                                          191 / 191 (100%)

Time: 17.39 seconds, Memory: 6.00Mb

There was 1 error:

1) StatementTest::testVerifyIncorrectPubKey
openssl_pkey_get_details() expects parameter 1 to be resource, boolean given

C:\Users\John\Projects\TinCanPHP\tests\StatementTest.php:828

--

There were 8 failures:

1) RemoteLRSTest::testSaveStatementsWithAttachments
success
Failed asserting that false is true.

C:\Users\John\Projects\TinCanPHP\tests\RemoteLRSTest.php:171

2) RemoteLRSTest::testRetrieveStatementWithAttachments
attachment content
Failed asserting that null is identical to '{"foo":"bar"}'.

C:\Users\John\Projects\TinCanPHP\tests\RemoteLRSTest.php:242

3) RemoteLRSTest::testRetrieveActivity
retrieved activity
'{"error":true,"success":false...ce"}]}' does not match expected type "object".

C:\Users\John\Projects\TinCanPHP\tests\RemoteLRSTest.php:481

4) RemoteLRSTest::testRetrievePerson
retrieved person
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 TinCan\Person Object (
     'objectType' => 'Person'
     'name' => Array (
-        0 => 'Example User'
     )
     'mbox' => Array (
-        0 => 'mailto:[email protected]'
     )
-    'mbox_sha1sum' => null
-    'openid' => null
-    'account' => null
+    'mbox_sha1sum' => Array ()
+    'openid' => Array ()
+    'account' => Array ()
 )

C:\Users\John\Projects\TinCanPHP\tests\RemoteLRSTest.php:541

5) StatementVariationsTest::testAttachmentsString
successful request
Failed asserting that false is true.

C:\Users\John\Projects\TinCanPHP\tests\StatementVariationsTest.php:116

6) StatementVariationsTest::testAttachmentsBinaryFile
Failed asserting that false is true.

C:\Users\John\Projects\TinCanPHP\tests\StatementVariationsTest.php:152

7) StatementVariationsTest::testSignedAndVerified
verify signature
Failed asserting that false is true.

C:\Users\John\Projects\TinCanPHP\tests\StatementVariationsTest.php:186

8) StatementVariationsTest::testSignedAndVerifiedX5c
verify signature
Failed asserting that false is true.

C:\Users\John\Projects\TinCanPHP\tests\StatementVariationsTest.php:220

FAILURES!
Tests: 191, Assertions: 700, Errors: 1, Failures: 8.


Non-exception error occurred:

2: openssl_x509_read(): supplied parameter cannot be coerced into an X509 certificate!
C:\Users\John\Projects\TinCanPHP\src\Statement.php (254)

Contents of test/Config.php:

<?php

$LRSs = [
    [
        'endpoint' => 'http://localhost:8000/data/xAPI/',
        'username' => '402da968f21dee8300df6cc7ded89dea5e3665e8',
        'password' => '142a594f5dbcdf8d5d693b27d92d46925a08ce95',
        'version'  => '1.0.1'
    ]
];
$KEYs = [
    'public' => __DIR__ . '/keys/travis/cacert.pem',
    'private' => __DIR__ . '/keys/travis/privkey.pem',
    'password' => 'travis',
];

The LRS credentials are from the instance of LearningLocker I have set up locally for testing our own LRS systems and I started this file from scratch and added the credentials directly from the web interface before running the tests, so I know they're correct. The learninglocker version is a bit out of date though, which may be contributing to this. I'll test this again with a more recent version of LL.

garemoko · 2016-01-12T16:51:23Z

@johnpbloch I imagine that these tests were originally designed against SCORM Cloud. Aside from actual bugs in whichever Learning Locker version you're using, there may be legitimate style issues that cause failed tests. For example on test 4, Learning Locker has returned empty arrays where the test expected the properties to be null. Either are arguably valid by the spec.

Ideally the tests should be made to work with any conformant LRS, but that's out of scope of this PR.

garemoko · 2016-01-12T16:52:18Z

Note: you can get a free SCORM Cloud account for testing here: https://cloud.scorm.com/sc/guest/SignUpForm

brianjmiller · 2016-01-13T16:08:36Z

You can also trigger a Travis build yourself on your own fork through their site which will run the tests as I will ultimately. Just beware that simultaneous test runs against the same Cloud account can be problematic, unfortunately the tests aren't safe that way.

WillSkates · 2016-01-14T14:00:55Z

EDIT: Thank you for the input everyone. If the tests pass successfully with Travis the library should work for anyone shouldn't it?

brianjmiller · 2016-01-14T14:06:14Z

If I understand what you are asking, not really, short of a bug in Travis which happens from time to time. That consistency is one of the big reasons to rely on Travis.

WillSkates · 2016-01-14T16:36:41Z

@brianjmiller Yeah sorry I'll rewrite that. Are you happy with the PR in general?

WillSkates · 2016-01-14T16:48:37Z

Apologies my hand slipped and I accidentally clicked close instead of comment.

garemoko · 2016-01-18T19:26:09Z

.gitignore

@@ -22,4 +22,4 @@ todo.md
 # eclipse project files
 .buildpath
 .project
-.settings
+.settings


Please avoid whitespace changes in files that haven't otherwise changed in order to avoid wrongly updating the "last updated" date.

This hasn't been addressed.

brianjmiller · 2016-08-22T20:18:18Z

src/RemoteLRS.php

+            'until', 'limit',
+            'format'
+        ];
+        foreach ($content as $k) {


Why make these separated out, and then change the original array construct for agent to be an inlined version? Does this change apply something for PHP7?

brianjmiller · 2016-08-22T20:39:47Z

Made a closer first pass. Isolating the changes needed for a specific context would make this kind of thing much simpler in the future, IOW there seem to be relatively few changes necessary to get actual PHP7 compatibility, which could have been isolated from the test coverage pieces, which should have definitely been isolated from subjective code changes. I'll continue to work on this tomorrow, I want to maintain history if you feel it is important. If you'd rather take on a task of splitting this work up, and/or squashing/rebasing then feel free, let me know if you plan to.

brianjmiller · 2016-08-23T15:36:43Z

tests/ActivityTest.php

        );
        $obj = Activity::fromJSON('');
    }

    public function testFromJSONInvalidMalformed() {
        $this->setExpectedException(
-            'InvalidArgumentException',
-            'Invalid JSON: ' . JSON_ERROR_SYNTAX
+            'TinCan\JSONParseErrorException'


While a better overall implementation this is a loss of granularity of the unit tests, we no longer test distinguishing malformed versus null/empty. We should maintain that level of detail.

Heh, on further inspection these tests weren't doing what they were originally thought to do anyways. JSON_ERROR_NONE actually ends up being '' which doesn't change the error message which is why they were passing to begin with.

WillSkates · 2016-08-24T08:49:26Z

Hi @brianjmiller. I'm fine to look at cleaning this up for you now that you are active here again. Closing this in favor of a 'cleaner' solution.

brianjmiller · 2016-08-24T13:43:01Z

@WillSkates 👍 .

I suspect you have, but if you haven't, take a look at #50, I'm planning to accumulate the bare minimum for PHP 7 support there. It leverages the new Exception from this PR though altered a little bit and committed differently (commit message credits you though) and the corresponding changes to test files related to that change. Everything else in here I believe can be brought over in a patch (or minor) version and mostly hinges around testing AFAICT.

Splitting up the remaining parts of this PR will help get them through more quickly, for instance, straight additions of new test files (so long as they conform to the rest of the style) should be easy to add as they can't affect functionality. Followed by other adjustments to tests, and then tests and code with the latter being the most dangerous and therefore comes more scrutiny. Isolating groups of changes make it far easier to review and tweak, multiple PRs is better than larger ones.

In any case, I look forward to your submissions and appreciate the work (and patience) so far.

WillSkates · 2016-08-24T13:57:26Z

Hi @brianjmiller,

Can you please set two milestones:

100% code coverage.
PHP7 support.

We can label my PR's against those.

Cheers,
Will

brianjmiller · 2016-08-24T14:13:00Z

I think the first is a lofty goal, though you are welcome to try. If we're that close then I'd seriously look at where the metric comes from. Having said that, happy to create a milestone.

#50 achieves the second, we're there, I'm looking at the namshi/jose issue (#61) a little today. But I plan to cut a last minor version with the existing changes on master and then merge #50 and cut the major release.

If you have something that you absolutely want in a major release (because it breaks backwards compat) better get it in very soon.

WillSkates · 2016-08-24T14:53:03Z

@brianjmiller I've already achieved it. I doubt #50 does it fully as the cc in that branch isn't sufficient. I'll see what I can get you when. If you want to push for a major version before then you can but I'd advise against it.

brianjmiller · 2016-08-24T17:41:18Z

Is there some reason to advise against it? Current tests pass and they "cover" the "difficult" parts which is the JSON serialization/deserialization, and HTTP requests which are the parts likely to break in PHP 7. AFAICT all other changes in the other PR are either tests or to facilitate tests of what amounts to existing code, and therefore by virtue of it passing so should the result of #50.

WillSkates · 2016-08-24T20:08:10Z

@brianjmiller I ran into places where I had to solve problems that were exposed by new tests. However I honestly can't remember them one by one so it isn't much to go on. I'll defer to you and I can work to improve your test coverage and solve them as we go in following minor versions.

Adding this to your local phpunit.xml.dist will give you a more accurate display of the code coverage as it is:

<log type="coverage-html" target="report" showUncoveredFiles="true" lowUpperBound="50" highLowerBound="80" />

brianjmiller · 2016-08-24T21:29:21Z

Added, in a manor of speaking, to #50, along with some other fixes to make coverage output correctly with latest PHPUnit.

brianjmiller · 2016-08-24T22:08:55Z

Heh, so I've gone through the bulk of the added files (all but one) and so far I've run into 3 issues, but none of which are related to PHP 7 support, they are just bugs that are in the 0.x series anyways, so by rights those will likely get fixed as part of it and I'll end up doing another patch release (though I hadn't planned to). I wish they'd been part of a separate PR! Oh well.

WillSkates · 2016-08-26T09:22:44Z

Yeah I definitely agree that they should have been in a separate PR. Your work to split this out is very much appreciated. I'd still like to have a chat on VOIP about the lib in general if you are willing seeing as we still haven't actually met but we've both put in a fair amount of effort on this. You have my email address anyway :D.

brianjmiller · 2016-08-27T12:48:55Z

A little research suggests the reason I felt "100% test coverage" was a lofty goal (and not yet achieved) and is summed up by this:

The Opcode Coverage, Branch Coverage, and Path Coverage software metrics are not yet supported by PHP_CodeCoverage.

So the coverage is basically line coverage (which is an okay metric, but not great for regression management in a theoretical zero backwards compatibility change use case). I still think it is a worthy goal, but it is really important to understand just what the coverage metric is giving us and to understand the type of risks still present when making extraneous changes.

Reference: https://phpunit.de/manual/current/en/code-coverage-analysis.html

WillSkates changed the title ~~Testing and adapting for php7 if necessary.~~ WIP Testing and adapting for php7 if necessary. Dec 16, 2015

WillSkates changed the title ~~WIP Testing and adapting for php7 if necessary.~~ [Ready]Testing and adapting for php7 if necessary. Dec 21, 2015

WillSkates closed this Jan 14, 2016

WillSkates reopened this Jan 14, 2016

garemoko reviewed Jan 18, 2016
View reviewed changes

brianjmiller reviewed Aug 22, 2016
View reviewed changes

garemoko mentioned this pull request Aug 23, 2016

fix fopen headers for php7 #72

Closed

brianjmiller reviewed Aug 23, 2016
View reviewed changes

WillSkates closed this Aug 24, 2016

brianjmiller mentioned this pull request Aug 25, 2016

Test Coverage improvement and various fixes #73

Merged

WillSkates mentioned this pull request Aug 26, 2016

Switch to using a mock HTTP request object for RemoteLRS unit tests #70

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Testing and adapting for php7 if necessary. (Including an increase in Code Coverage) #47

[WIP] Testing and adapting for php7 if necessary. (Including an increase in Code Coverage) #47

WillSkates commented Dec 16, 2015

garemoko commented Dec 16, 2015

WillSkates commented Dec 16, 2015

WillSkates commented Dec 21, 2015

garemoko commented Dec 28, 2015

WillSkates commented Jan 11, 2016

garemoko commented Jan 11, 2016

johnpbloch commented Jan 11, 2016

johnpbloch commented Jan 11, 2016

WillSkates commented Jan 12, 2016

johnpbloch commented Jan 12, 2016

garemoko commented Jan 12, 2016

garemoko commented Jan 12, 2016

brianjmiller commented Jan 13, 2016

WillSkates commented Jan 14, 2016

brianjmiller commented Jan 14, 2016

WillSkates commented Jan 14, 2016

WillSkates commented Jan 14, 2016

garemoko Jan 18, 2016

brianjmiller Feb 10, 2016

brianjmiller Aug 22, 2016

brianjmiller commented Aug 22, 2016

brianjmiller Aug 23, 2016

brianjmiller Aug 23, 2016

WillSkates commented Aug 24, 2016

brianjmiller commented Aug 24, 2016

WillSkates commented Aug 24, 2016

brianjmiller commented Aug 24, 2016

WillSkates commented Aug 24, 2016

brianjmiller commented Aug 24, 2016

WillSkates commented Aug 24, 2016

brianjmiller commented Aug 24, 2016

brianjmiller commented Aug 24, 2016

WillSkates commented Aug 26, 2016

brianjmiller commented Aug 27, 2016

[WIP] Testing and adapting for php7 if necessary. (Including an increase in Code Coverage) #47

[WIP] Testing and adapting for php7 if necessary. (Including an increase in Code Coverage) #47

Conversation

WillSkates commented Dec 16, 2015

garemoko commented Dec 16, 2015

WillSkates commented Dec 16, 2015

WillSkates commented Dec 21, 2015

garemoko commented Dec 28, 2015

WillSkates commented Jan 11, 2016

garemoko commented Jan 11, 2016

johnpbloch commented Jan 11, 2016

johnpbloch commented Jan 11, 2016

WillSkates commented Jan 12, 2016

johnpbloch commented Jan 12, 2016

garemoko commented Jan 12, 2016

garemoko commented Jan 12, 2016

brianjmiller commented Jan 13, 2016

WillSkates commented Jan 14, 2016

brianjmiller commented Jan 14, 2016

WillSkates commented Jan 14, 2016

WillSkates commented Jan 14, 2016

garemoko Jan 18, 2016

Choose a reason for hiding this comment

brianjmiller Feb 10, 2016

Choose a reason for hiding this comment

brianjmiller Aug 22, 2016

Choose a reason for hiding this comment

brianjmiller commented Aug 22, 2016

brianjmiller Aug 23, 2016

Choose a reason for hiding this comment

brianjmiller Aug 23, 2016

Choose a reason for hiding this comment

WillSkates commented Aug 24, 2016

brianjmiller commented Aug 24, 2016

WillSkates commented Aug 24, 2016

brianjmiller commented Aug 24, 2016

WillSkates commented Aug 24, 2016

brianjmiller commented Aug 24, 2016

WillSkates commented Aug 24, 2016

brianjmiller commented Aug 24, 2016

brianjmiller commented Aug 24, 2016

WillSkates commented Aug 26, 2016

brianjmiller commented Aug 27, 2016