Make WordPress Core

Opened 8 years ago

Closed 3 years ago

#34694 closed enhancement (duplicate)

Facilitate automated testing in context of pull requests and diffs

Reported by: westonruter's profile westonruter Owned by: westonruter's profile westonruter
Milestone: Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords:
Focuses: Cc:

Description (last modified by westonruter)

Using GitHub for core development has been a wishlist item for awhile now. Individual feature plugins have been increasingly developed on GitHub (e.g. Widget Customizer, WP-API). Going an extra step, the Twenty Sixteen theme only exists on GitHub and isn't even committed to trunk. So while feature plugins and themes are often being developed primarily on GitHub, other work on Core is generally not (although this may change).

While there is no official wordpress-develop Git repo on GitHub (yet), anyone can clone the wordpress-develop Git repo and push it to GitHub. Teams working on various components can then work in the context of those GitHub clones, and then when work is complete a committer can apply the patch to SVN. Travis CI is already configured in the wordpress-develop repo, and so anyone who clones the repo to GitHub can automatically turn on Travis CI to start getting automated build checks for each commit.

Work on GitHub is usually done in the context of pull requests, and Travis CI can be especially useful when running in response to a pull request being opened or a commit pushed to a feature branch with an existing pull request. As reported in #30017, many automated tests are unnecessarily slow. This is in large part because with each commit, _all_ checks are being made across all files in the project, irrespective of whether they were recently modified or not.

When Travis CI does a build in the context of a pull request, it makes available the branch checked out and the branch being merged into, and with these two refs in hand, we can gather a list of the files (and patches) that were specifically changed. When we have these, we can optimize Travis CI to only run the checks that are relevant to the changes in the feature branch. For instance, if no PHP files were changed, then all of the jobs that run PHPUnit tests can be cancelled. If no JS files were modified, then the JSHint checks can be skipped.

It can be painful to introduce new automated checks to WordPress because they can add a lot of noise for automated tests and fixing requires a touching a lot of files, possibly invalidating many pending patches. When Travis runs in the context of a pull request (or when automated tests are run when trunk/master is not checked out), then again, there is a diff available for the specific changes. We can introduce PHP_CodeSniffer for core (#30153) when Travis runs in in the context of a pull request, and it can skip reporting errors on any lines that aren't modified in the commit. This is likewise how ESLint can be added (#31823).

For existing code which causes Travis CI to only report coding standard violations to lines changed, see https://github.com/xwp/wp-dev-lib/blob/master/check-diff.sh when $CHECK_SCOPE is patches.

For commits pushed to master on Travis CI (outside the context of a pull request), it may make sense to configure Travis to check for changes made since the last major release.

Attachments (2)

xwp.wordpress-develop.34694.pr135.f89c25a4...7ea00442.diff (2.7 KB) - added by xwp-bot 8 years ago.
PR 135, ±Δ f89c25a4...7ea00442, Tests: ✅ PASS * Allow Travis CI to run user-defined external scripts via environment vars (by Weston Ruter)
xwp.wordpress-develop.34694.pr135.7ea00442...d13d9f40.diff (2.6 KB) - added by xwp-bot 8 years ago.
PR 135, ±Δ 7ea00442...d13d9f40, Tests: ✅ PASS * Remove extra semicolons (by Weston Ruter) * Temporarily cache-bust external requests (by Weston Ruter) * Run custom scripts before other scripts (by Weston Ruter) * Remove cache busting for custom scripts (by Weston Ruter)

Download all attachments as: .zip

Change History (20)

#1 @westonruter
8 years ago

  • Description modified (diff)

#2 follow-up: @jorbin
8 years ago

I've started on a script to accomplish this in the short term with travis and github. How it works:

1) Fetches the rss of changes to trac.
2) Finds all the uploaded patches.
3) Creates a new Branch for each patch
4) Applies that patch using grunt patch
5) pushes that branch to github

Right now it can fail due to patches not being generated from the root or otherwise failing to apply. grunt patch will need to have an optional argument added to fail if a patch can't be applied instead of prompting the user to manually specificy a location (PR welcome if someone can get to this before me). The script looks like this so far:

#!/bin/bash

# Set DIR based on the current Directory
DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

# Get a list the 50 most recent changes
curl  "https://core.trac.wordpress.org/timeline?ticket_details=on&max=50&authors=&daysback=90&format=rss" |  \
# find the titles
grep --line-buffered "<title>\(.\)*</title>" | \ 
# Find the attachments
grep --line-buffered "attached" | \ 
# Remove the title tags
awk '{gsub("<[^>]*>", "")}1' | \ 
# Create a attachment url
awk -F ' ' '{print "https://core.trac.wordpress.org/raw-attachment/ticket/" $5 "/" $1 }' | \ 
# Find only diff and patch files
grep --line-buffered "patch\|diff" | \ 
# remove the # from the ticket number in the url
awk '{gsub( "#", "")}1' > .tmp.urls

while IFS='' read -r line || [[ -n "$line" ]]; do
    BRANCHNAME=$( cut -d '/' -f 6-7 <<< $line ) 
    echo $BRANCHNAME
    if grep $BRANCHNAME  .patched ; then
        echo "Yo"
    else
        echo "Applying $line" 
        cd $DIR
        cd git-clone
        git checkout master
        git pull origin master
        npm install
        git checkout -b $BRANCHNAME
        grunt patch:$line
        git add -A
        git commit -m "add change from $line"
        cd $DIR
        echo $BRANCHNAME >> .patched
    fi  
done < .tmp.urls
Last edited 8 years ago by westonruter (previous) (diff)

#3 in reply to: ↑ 2 @westonruter
8 years ago

Replying to jorbin:

I've started on a script to accomplish this in the short term with travis and github.

Cool!

Ideas:

  • Attribute the author in the commit, e.g. via --author "Full Name <username@git.wordpress.org>"
  • Use one branch per ticket, and apply all of the patches from all authors sequentially. This allows change history of patches to be easily seen, with each new contributor's patch changes clearly displayed with each git commit. I came up with a prototype: https://gist.github.com/westonruter/5c453a44bd36f6847c36
  • Open a pull request to master for each ticket branch.
  • Add a comment to the Trac ticket with the URL to the pull request when it is created for that ticket.
  • Add a comment to the Trac ticket when there is a Travis build failure.
Last edited 8 years ago by westonruter (previous) (diff)

@xwp-bot
8 years ago

PR 135, ±Δ f89c25a4...7ea00442, Tests: ✅ PASS

  • Allow Travis CI to run user-defined external scripts via environment vars (by Weston Ruter)

@xwp-bot
8 years ago

PR 135, ±Δ 7ea00442...d13d9f40, Tests: ✅ PASS

  • Remove extra semicolons (by Weston Ruter)
  • Temporarily cache-bust external requests (by Weston Ruter)
  • Run custom scripts before other scripts (by Weston Ruter)
  • Remove cache busting for custom scripts (by Weston Ruter)

#4 @westonruter
8 years ago

I have put together an initial version of a set of scripts that handle the sync from GitHub pull requests to WordPress Trac via Travis CI: https://github.com/xwp/wp-github-pull-request-travis-ci-trac-sync

See the attachments for a working demonstration of what the scripts do.

For more information, see the readme and the full writeup in Streamlining Contributions to WordPress Core via GitHub.

Caveat: This is intended for users in trusted contributor teams as it requires the user to open open an internal (intra-repo) pull request from feature branch to master. It will not work when opening a pull request from a fork due to Travis CI's security restrictions on environment variables. So any pull requests from external contributors will need to be manually applied to a feature branch by a repo contributor and then open an intra-repo pull request (while closing the original inter-repo pull request).

Last edited 8 years ago by westonruter (previous) (diff)

#5 @westonruter
8 years ago

  • Milestone changed from Future Release to 4.5

#6 @westonruter
8 years ago

  • Owner set to jorbin
  • Status changed from new to reviewing

@jorbin The patch here is specifically for facilitating forks on GitHub to integrate their own extensions to how Travis CI runs, such as integrating with an external bot to upload the patches to Trac. The demonstrated extension also add PHP_CodeSniffer and JSCS checks to the diff between master and the feature branch in the pull request. _But_ the code required for this (the class-git-patch-checker.php and related scripts) can be put into Core tools/. When Travis runs inside the context of a pull request, it can do the coding standards checks on the diff between the feature branch and master. But, when Travis runs outside the context of a pull request, what we can do is use the latest stable release tag (e.g. 4.4.0) as the start commit for the range of commits to check coding standards on. This will allow us to release-by-release enforcing coding standards via CI checks.

#7 @westonruter
8 years ago

Related aside: I just saw jQuery UI's mention-bot which automatically detected contributors who had touched the lines being impacted by a pull request, to ping them for code reviewing the PR: https://github.com/jquery/jquery-ui/pull/1663#issuecomment-171847209

This ticket was mentioned in Slack in #core by jorbin. View the logs.


8 years ago

#9 @jorbin
8 years ago

  • Milestone changed from 4.5 to WordPress.org

This Doesn't need to be tied to a specific release.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


8 years ago

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


8 years ago

#13 @westonruter
7 years ago

  • Owner changed from jorbin to westonruter
  • Status changed from reviewing to accepted

This ticket was mentioned in Slack in #core by westonruter. View the logs.


7 years ago

#15 @westonruter
7 years ago

  • Description modified (diff)

This ticket was mentioned in Slack in #core-editor by netweb. View the logs.


7 years ago

This ticket was mentioned in Slack in #core-js by netweb. View the logs.


7 years ago

#18 @desrosj
3 years ago

  • Milestone WordPress.org deleted
  • Resolution set to duplicate
  • Status changed from accepted to closed

With GitHub pull requests now officially being a blessed way to contribute to Core, and the introduction of GitHub Actions workflows in #50401, I think this ticket has been solved elsewhere.

The GitHub Action workflows (which check PHP/JS coding standards, PHPUnit tests, JS tests, etc.) run for every pull request, and Travis will soon be removed.

I'm going to mark this as a duplicate of #50401 as that ticket will directly accomplish the goal of running automated checks/testing in the context of pull requests.

Note: See TracTickets for help on using tickets.