configlet icon indicating copy to clipboard operation
configlet copied to clipboard

sync: warn that syncing docs or tests alone may be problematic

Open ee7 opened this issue 2 years ago • 0 comments

We should draw more attention to the fact that, when an exercise has outdated docs and tests, updating only one of them is not guaranteed to be safe. Arguably, the design of configlet sync, with separate --docs and --tests options, implies that it is.

For example, see these roman-numerals PRs that update independently:

  • https://github.com/exercism/common-lisp/pull/676#pullrequestreview-1152375062
  • https://github.com/exercism/tcl/commit/6b1eae5ef421395346867cb5b910207b389980e6 with earlier https://github.com/exercism/tcl/commit/22ffade3385244abac480f333bf3948b0cf88fef (less of a problem, because docs-before-tests was better than tests-before-docs in this case)

Instead, the docs and tests should be updated in a single commit, like https://github.com/exercism/ruby/commit/d3cb76992f13cf5adf72923b71f5467b02fcd511.

The above example isn't so bad, but there's potential for a severe mismatch if:

  • We overhaul an exercise completely in prob-specs, so that both the docs and tests are heavily changed.
  • Then we merge a PR in a track repo that updates only the docs or tests for that exercise.
  • Then there is a large delay in creating, reviewing, or merging a PR that updates the other thing for that exercise.

I think the most robust workflow for syncing is:

  1. One PR per exercise that has unsynced docs and tests, updating both in the same PR.
  2. Then, one PR per exercise that has only unsynced tests.
  3. Then a single PR to update the docs for all remaining exercises (those that only have unsynced docs).

Then all the PRs can always be merged independently.

I suggest that we should eventually have a configurable bot that opens PRs in such a fashion, defaulting to including all new test cases. But for now, maintainers should keep this in mind when creating PRs and reviewing.

ee7 avatar Oct 24 '22 13:10 ee7