Skip to content

Commit ac7f6b7

Browse files
committed
Address corpus-diff review notes
- Name the comparison for what it is: on pull_request HEAD is the PR merged into the base branch, so the merge base resolves to the base branch tip. Rename the step, explain it in the workflow, and say the same in the README, including how to reproduce that comparison locally. - Drop the unreachable is_array() guard in tools/export-corpus.php: get_wp_files() returns a WP_Error on failure, which does not exist under plain PHP, so that path fatals with a non-zero exit before it can return. - Add a concurrency group so a new push cancels the in-flight run for the same PR.
1 parent fd8ea4e commit ac7f6b7

3 files changed

Lines changed: 22 additions & 11 deletions

File tree

‎.github/workflows/corpus-diff.yml‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
1-
# Parses a pinned corpus of WordPress core source with the parser at the PR's
2-
# merge base and at its head, normalizes both JSON outputs with prep-diff.php,
3-
# and diffs them. Anything in the diff is a behavior change this PR makes:
4-
# every hunk must be either intended (and explained in the PR) or a regression.
1+
# Parses a pinned corpus of WordPress core source with the parser at the base
2+
# branch and with the PR merged into it, normalizes both JSON outputs with
3+
# prep-diff.php, and diffs them. Anything in the diff is a behavior change this
4+
# PR makes: every hunk must be either intended (and explained in the PR) or a
5+
# regression.
56
#
67
# Policy decisions, deliberate:
78
# - The corpus is pinned to one WordPress tag so diffs are reproducible.
@@ -19,6 +20,11 @@ name: Corpus Diff
1920
on:
2021
pull_request:
2122

23+
# A new push to the PR supersedes any run still in flight for it.
24+
concurrency:
25+
group: ${{ github.workflow }}-${{ github.event.pull_request.number }}
26+
cancel-in-progress: true
27+
2228
jobs:
2329
corpus-diff:
2430
name: WordPress corpus regeneration diff
@@ -65,7 +71,12 @@ jobs:
6571
echo "Corpus PHP files: ${count}"
6672
[ "$count" -ge 500 ]
6773
68-
- name: Check out merge base
74+
# On pull_request, HEAD is the PR merged into the base branch, so its
75+
# merge base with the base branch is the base branch tip, not the commit
76+
# the PR branched from. That is the intended comparison: base as it is
77+
# now against base plus exactly this PR, with no hunks from other work
78+
# that landed since the branch point.
79+
- name: Check out base
6980
run: git worktree add base "$(git merge-base "origin/${{ github.base_ref }}" HEAD)"
7081

7182
- name: Install Composer dependencies (head)

‎README.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ wp parser create /path/to/source/code --user=<id|login>
5353

5454
## Corpus diff
5555

56-
Unit tests do not cover every shape of real-world documentation, so changes to the parser are also checked against a corpus of WordPress core source. The same corpus is parsed with the parser at two refs — the merge base of a pull request and its head — both JSON outputs are normalized with `prep-diff.php`, and the two are diffed. Everything in that diff is a behavior change the pull request makes: every hunk must be either intended and explained, or it is a regression.
56+
Unit tests do not cover every shape of real-world documentation, so changes to the parser are also checked against a corpus of WordPress core source. The same corpus is parsed with the parser at two refs — the base branch and the pull request merged into it — both JSON outputs are normalized with `prep-diff.php`, and the two are diffed. Everything in that diff is a behavior change the pull request makes: every hunk must be either intended and explained, or it is a regression.
5757

5858
`.github/workflows/corpus-diff.yml` runs this on every pull request. The corpus is `wp-includes` from a pinned WordPress tag (`WP_CORPUS_TAG` in the workflow), so diffs are reproducible. The job is non-blocking: it uploads the diff as a `corpus.diff` artifact and reports the hunk count in the job summary. The head checkout's `tools/export-corpus.php` and `prep-diff.php` drive both sides, so tooling changes never masquerade as parser changes; when `prep-diff.php` itself changes, its effect on normalization shows up in the diff and is reviewed like any other change.
5959

@@ -72,6 +72,8 @@ composer --working-dir=base install
7272
composer install
7373
```
7474

75+
On a branch this compares against the commit the branch left `master` at. CI runs on the pull request merged into `master`, so it compares against the `master` tip; merge `master` into the branch first to get the same comparison.
76+
7577
Export both sides over the same corpus, normalize, and diff. `export-corpus.php` takes the parser root and the corpus directory, and writes JSON to stdout:
7678

7779
```bash

‎tools/export-corpus.php‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,9 @@
3838

3939
require $parser_root . '/vendor/autoload.php';
4040

41+
// get_wp_files() reports an unreadable subdirectory with a WP_Error, a class
42+
// that only exists inside WordPress: under plain PHP that path is a fatal
43+
// error with a non-zero exit, so there is no return value to check here.
4144
$files = \WP_Parser\get_wp_files( $corpus_dir );
4245

43-
if ( ! is_array( $files ) ) {
44-
fwrite( STDERR, 'Could not list the PHP files in ' . $corpus_dir . '.' . PHP_EOL );
45-
exit( 1 );
46-
}
47-
4846
echo json_encode( \WP_Parser\parse_files( $files, $corpus_dir ), JSON_PRETTY_PRINT ), PHP_EOL;

0 commit comments

Comments
 (0)