Make optional limit on Fetcher consistent

[whikloj]

The getRecords() function of the Fetcher class, says it is optional so I think the null default used in the Csv implementation should be added in the Fetcher class function signature. That means that the Cdm class would also need to add it to its implementation to be consistent but it doesn't have to use it.

Also, if you want me to submit PRs for these changes I am suggesting or if you just want me to quiet down. Let me know 😜

[mjordan]

@whikloj thanks for pointing this out. We welcome PRs ❤️ Your #400, and #406, #407, #414, and the now-closed #25 and #365 are all along the same lines.... we want to clean up our code. One obstacle we have is that we don't have a way to apply PHPUnit tests to entire CONTENTdm toolchains, and we don't have access to a live CONTENTdm instance any more either. So, we don't really have a good way of testing for regression bugs. However, we can apply PHPUnit tests to the CONTENTdm fetcher so if you want to open PR for the Cdm class, go for it! If you do, please indicate the changes are 'testable' and provide how many tests and assertions are expected , e.g., "You should get 52 tests, 72 assertions." (note that the number of tests and assertions can be the same as pre-change tests, if the change is invisible, like #405).

[whikloj]

@mjordan I can't run your tests in PHP 7 (in a clean git clone phpunit explodes) and for my class I'm using PSR-4 which requires altering the composer.json autoload section. So I'm not sure if at this point I am just diverging from the existing codebase too much.

[MarcusBarnes]

@mjordan We're actually requiring (targeting) an unsupported version of PHPUnit https://github.com/MarcusBarnes/mik/blob/master/composer.json#L10. We may want to consider adjusting the tests to run in the current old stable release 5.7 to get support for PHP 5.6, PHP 7.0, and PHP 7.1. This would mean dropping support for PHP < 5.6. See https://phpunit.de/ for more details. However, we should consider this carefully, since folks may be using MIK to migrate to newer systems so that they can actually get ride of older servers running older versions of PHP, etc.

[mjordan]

Well, we shouldn't be requiring an unsupported version of PHPUnit, that's for sure. I see no problem in moving our minimum PHP version to 5.6 if that will allow us to fix that. At least that will let us use the "Old stable release" of PHPUnit.

[mjordan]

@MarcusBarnes good point::

However, we should consider this carefully, since folks may be using MIK to migrate to newer systems so that they can actually get ride of older servers running older versions of PHP, etc.

If PHPUnit's PHP requirements are the biggest issue, is there a way to specify one PHP minimum version for development and another (lower) version for non-development environments?

[whikloj]

I think the problem I am having is that the tests don't use the namespaced version of the classes. Is that what you mean @MarcusBarnes by the older version?

[MarcusBarnes]

@whikloj The PHPUnit version installed by default when using composer is PHPUnit 4.8, which doesn't cover PHP 7. There's also non-trivial differences between PHPUnit 4, 5, and 6. I'm assuming that you probably have PHPUnit globally installed on your system and that since you're running PHP 7, that you're running PHPUnit 5 or 6. Is that correct?

[whikloj]

@MarcusBarnes yep 6.2.2

[whikloj]

So here is what I am going to do, I'll work on migrating some of the tests to work with PHPUnit 6.2.2. I'll leave this in a separate branch on my repo. Once you guys are ready, you could fork my repo and push the branch up to yours as a starting point for your PHP 7 migration.

I will warn you that I also made a fetcher called FileFetcher though 😄

[mjordan]

@whikloj and @MarcusBarnes I'm missing something. Our .travis.yml file specifies PHP 7 and the PHPUnit tests pass. Is that because when Travis runs it tests in PHP 7, it is running a newer version of PHPUnit, like 6.x?

[whikloj]

@mjordan that is a good question, that is the same version I am running maybe its the inputvalidator exclusion that causes mine to explode

[whikloj]

@mjordan actually its because there are no tests executed

https://travis-ci.org/MarcusBarnes/mik/jobs/247984075#L242 https://travis-ci.org/MarcusBarnes/mik/jobs/247984075#L253

[mjordan]

Hm. The mystery deepens. If no tests are executed (which is is what it says), why do https://travis-ci.org/MarcusBarnes/mik/jobs/247984075#L245 and https://travis-ci.org/MarcusBarnes/mik/jobs/247984075#L256 exist?

Also, I'm pretty sure I've pushed up commits that have caused the build to fail because of failed PHPUnit tests. Might be interesting to test that, to determine which of the seemingly contradictory entries in the travis log are inaccurate.

[whikloj]

Not sure I understand what you mean by "exist", but essentially if no tests failed then you pass. 😄

So as no tests were run there were no failures. The tests are still there, just that PHPUnit 6 is not seeing them. With my new test, I can run your testing command and see:

> phpunit --exclude-group inputvalidators --bootstrap vendor/autoload.php tests
PHPUnit 6.2.2 by Sebastian Bergmann and contributors.

.........                                                           9 / 9 (100%)

Time: 135 ms, Memory: 10.00MB

OK (9 tests, 14 assertions)

[whikloj]

@mjordan this is exactly the problem that @jordandukart and...someone (@qadan maybe?) figured out for Islandora core. How do you run old and new PHPUnit to cover old and new versions of php. Perhaps he can provide you with some suggestions?

[mjordan]

Fair enough, I was assuming that if no tests ran, PHPUnit wouldn't exit with a 0.