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

Simplify pull request workflow for testable changes #384

Closed mjordan closed 7 years ago

mjordan commented 7 years ago

As discussed in #370, we should introduce a new workflow that allows work that is covered by PHPUnit tests to be merged, at the discretion of the reviewer, without a full smoketest. The intent of this change is to reduce the burden on the person opening the PR to provide sample configuration and data to perform the smoke test. The proposed new workflow is:

  1. Person working on issue must incorporate the change into one of the existing PHPUnit tests, or provide new tests as applicable.
  2. Person working on the issue must: a. state in the PR template that the tests pass on their local dev copy, b. summarize how the tests apply to the code changes in the PR, and c. indicate the expected number of successful tests and assertions.
  3. Person reviewing PR clones branch, and runs the tests.
  4. If the tests pass on the reviewer's local copy, and the reviewer agrees that the test code does in fact covers the code changes, the reviewer can decide if they want to merge into master without performing further smoke tests. Reviewer also has the option of deciding that the tests are not sufficient or that a smoke test involving sample data and configuration is justified.
  5. If the person working on the issue does not provide a PHPUnit test, a smoke test is required prior to merging (i.e., the current workflow applies).

Once we finalize this new workflow, we will need to update our CONTRIBUTING.md.

mjordan commented 7 years ago

@MarcusBarnes I'm thinking ahead a bit to how we want to describe this in a revised CONTRIBUTING.md. Can we call this new workflow the "testable" workflow, and call the other one the "smoketest" workflow, just to distinguish them? Can you think of other labels we might give them?

mjordan commented 7 years ago

Also, reminder to also update the PR template as per https://github.com/MarcusBarnes/mik/pull/386#issuecomment-304357684.

mjordan commented 7 years ago

@MarcusBarnes now that we've used the "testable" workflow in #386 and #390, are you comfortable officially adopting it? I guess by that I mean updating the CONTRIBUTING.md to describe it.

MarcusBarnes commented 7 years ago

Let's officially adopt the testable workflow for appropriate pull-requests.

mjordan commented 7 years ago

OK, I'll open a PR updating the CONTRIBUTING.md and PR template.

MarcusBarnes commented 7 years ago

Addressed in pull-request https://github.com/MarcusBarnes/mik/pull/394 (merged with commit https://github.com/MarcusBarnes/mik/commit/174be7b2f986fa2a17b1f6f0b9d8b37a823fb61c).