search

MarcusBarnes / mik

The Move to Islandora Kit is an extensible PHP command-line tool for converting source content and metadata into packages suitable for importing into Islandora (or other digital repository and preservations systems).
GNU General Public License v3.0
34 stars 11 forks source link

Psr2 and tests #424

Closed whikloj closed 7 years ago

whikloj commented 7 years ago

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. (#277) 2) Moves to use namespaced PHPUnit classes to support PHPUnit 6. (#419) 3) Make MetadataParser an abstract class with the metadata function. (#406) 4) Ensures all tests avoid using the static caching functions of fetchers and filegetters to avoid stomping over each other. (#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:

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 commented 7 years ago

Codecov Report

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

Impacted file tree graph

@@            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 commented 7 years ago

@whikloj thanks! WRT testing, I propose:

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

MarcusBarnes commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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

mjordan commented 7 years ago

Super, thanks!

MarcusBarnes commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

mjordan commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

Sounds good to me! Go for it!

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