Psr2 and tests

[whikloj]

Github issue: #417 #277 #419 (this also subsumes PR #420 and issue #406 )

What does this Pull Request do?

It:

1. Gets codebase into PSR-2 compliance. (Start reviewing code for compliance with PSR2 #277)
2. Moves to use namespaced PHPUnit classes to support PHPUnit 6. (Move to support PHPUnit 6 tests #419)
3. Make MetadataParser an abstract class with the metadata function. (Add abstract method ->metadata() to MetadataParser.php #406)
4. Ensures all tests avoid using the static caching functions of fetchers and filegetters to avoid stomping over each other. (Static caching bleeding in tests #417)

What's new?

Lots, sooooo much.

A lot of code style fixes to work with PSR-2, re-organization of the tests and the addition of a new Testing Base Class (MikTestBase), changes to all tests to define 'use_cache' = > true when ever defining FETCHER or FILEGETTER configuration. Also ensure that all fetchers and file_getters that use a static cache skip it with 'use_cache' => false.

How should this be tested?

This is alot, not sure how to make it clean. Tests should still work.

To help reviewers test your work:

• Indicate whether your work requires a smoke test or is covered by PHPUnit tests (see CONTRIBUTING.md for more information).

  Work is covered by unit tests, but testing with any handy config lying around might make sense.

• If your work is covered by PHPUnit tests, indicate how many successful tests and assertions the reviewer should see when they run your tests.

  I am seeing

OK, but incomplete, skipped, or risky tests!
Tests: 56, Assertions: 86, Skipped: 1.

• If your work requires a smoke test, provide sample configuration files and data for reviewers.

  This is a tough call, not sure what is the best way to ensure stability with this PR.

• Be as detailed as possible.

• Good testing instructions and sample confiruation files/data help get your PR completed faster.

Additional Information

This also outputs code coverage data, if @MarcusBarnes wants to setup the mik repo for use with http://codecov.io then you can see what parts of your code are actually tested by the existing tests and then work to cover the other parts in future.

Interested parties

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@0bd2a77). Click here to learn what that means.
The diff coverage is 18.25%.

@@            Coverage Diff            @@
##             master     #424   +/-   ##
=========================================
  Coverage          ?   27.54%           
=========================================
  Files             ?       79           
  Lines             ?     4389           
  Branches          ?        0           
=========================================
  Hits              ?     1209           
  Misses            ?     3180           
  Partials          ?        0

Impacted Files	Coverage Δ
src/metadataparsers/MetadataParser.php	100% <ø> (ø)
src/metadatamanipulators/MetadataManipulator.php	0% <ø> (ø)
src/config/CdmConfig.php	0% <ø> (ø)
src/filemanipulators/FileManipulator.php	0% <ø> (ø)
src/metadataparsers/json/Json.php	0% <ø> (ø)
src/metadataparsers/dc/Dc.php	0% <ø> (ø)
src/filegetters/CsvBooks.php	14.03% <0%> (ø)
src/filemanipulators/ThumbnailFromCDM.php	0% <0%> (ø)
src/filegetters/CdmNewspapers.php	0% <0%> (ø)
src/metadatamanipulators/PiratizeAbstract.php	0% <0%> (ø)
... and 64 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bd2a77...d3894df. Read the comment docs.

[mjordan]

@whikloj thanks! WRT testing, I propose:

• Generating sample input data using https://github.com/MarcusBarnes/mik/tree/master/extras/scripts/samplecontentgenerator, running MIK on the output, and then running https://github.com/mjordan/iipqa against the output. I've already got the start of script to automate this for all groups of Islandora content models (single-file, book, newspaper, compound)
• Running MIK with sample configs and data that @MarcusBarnes and I have supplied with various pull requests over the last couple months.
• Running MIK with some production configs and data that I still have lying around from various jobs here at SFU over the last couple of months

@MarcusBarnes any thoughts? I'd confident that if there are any major problems, all of this smoke testing will reveal them.

[MarcusBarnes]

@whikloj @mjordan Very pleased with all this constructive feedback as it will help improve MIK. I'm happy with the testing approach you propose @mjordan.

[mjordan]

@MarcusBarnes since you tested most of the recent PRs for which there are sample configs and data, maybe you could rerun those tests using @whikloj's PR branch?

[MarcusBarnes]

Yes - I'll schedule some time later today (or tomorrow morning) to start testing the PR.

[mjordan]

Super, thanks!

[MarcusBarnes]

Confirming that when running phpunit in the root directory without any additional options, I get the same result as @whikloj Tests: 56, Assertions: 86, Skipped: 1.

[mjordan]

I've got a script that (optionally) clones and installs MIK and IIPQA, uses MIK to generate sample input for single file, compound, book, and newspaper content models, runs MIK on this sample input, and then runs IIPQA on MIK's output. @MarcusBarnes this script is related to our discussion in #385.

The script works on both current master and @whikloj's new_master with a few modifications to MIK (bugs that slipped past in PR #398). These bugs are:

1. The mappings files should map "Date taken" to <dateIssued>, not <dateCreated>
2. The .ini Twig template should add shutdownhooks[] = 'php extras/scripts/shutdownhooks/create_structure_files.php' to .ini files generated for compound objects
3. In the CsvBooks writer, there is some copypasta cruft from the CsvNewspapers writer that needs to be removed.

I'm attaching the script for your inspection. I propose that:

1. We apply the fixes above to both master and @whikloj's new_master branches
2. We add the attached script to MIK's extras/scripts directory
3. Given that IIPAQ finds no problems with MIK output generated from both branches, we continue smoke tests on the new_master branch with additional configs/data as discussed above and if we encounter no issues, consider the changes in new_master to be stable enough to merge into master.
4. Prior to merging, we discuss cutting a 0.9.0 tagged release.

mik_integration_tests.php.txt

[MarcusBarnes]

@mjordan I'm good with proceeding with your proposal. Thank you. N.B. - I ran MIK using the CDM Singlefile toolchain configuration that I used for testing the fix to issue #410 against this pull-requests and it worked as expected.

[whikloj]

@mjordan @MarcusBarnes if you all want to make your bug fixes against master, I can rebase this PR on top of those changes.

[mjordan]

@whikloj thanks - why don't I open a PR that implements these fixes and adds the mik_integration_tests.php, and if you can rebase those changes in, that would be great. @MarcusBarnes sound good?

[mjordan]

@whikloj if you don't have time to do this, I don't mind rebasing or otherwise catching up on your branch this weekend and opening a separate PR, crediting you as the commit --author.

[whikloj]

@mjordan it appears no rebase is required (normally Github won't allow you to merge here), I looked over your commits on #426 and #428 and the only file we both touch is CsvBooks.php and we don't touch any of the same lines.

[mjordan]

@whikloj I just confirmed this by creating a new branch off of master and merging your psr2-and-tests into it, with no hassle from git. Sweet! PHPUnit shows only one skipped test, and running mik_integration_tests.php on your branch shows no problems. Beautiful.

@MarcusBarnes if we merge this into master, are there any other issues we want to resolve before tagging 0.9.0? Maybe #423, which I could do over the weekend?

[MarcusBarnes]

@mjordan I'm happy to merge and create a v0.9.0 release. If there are any other issues that should be resolved before the next major or minor release, we can address them and create an an appropriate patch release (e.g. v0.9.1).

Thanks so much @whikloj and @mjordan for working on these pull-requests.

[mjordan]

Sounds good to me! Go for it!

@whikloj I echo @MarcusBarnes's thanks, cleaning up our code has put us way ahead. Thanks!