grunt precommit should run phpunit tests

[ericlewis]

The grunt precommit task, as it says "Runs front-end dev/test tasks in preparation for a commit." phpunit is a test task, we should run it too.

[ocean90]

Related discussion: ​https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2014-02-13&sort=asc#m791485

[ericlewis]

@helen had a great point in that discussion:

if i'm making CSS changes i don't want to run [phpunit] tests, but that's specific to those commits :)

Perhaps our precommit task could check what files have been modified and decide what should be run. e.g.

• if css has been modified run postcss:core
• if js has been modified run browserify, jshint:corejs, uglify:bookmarklet, and qunit:compiled
• if php has bee modified run phpunit.

[iseulde]

This is the reason I didn't see this coming. ​https://travis-ci.org/aaronjorbin/develop.wordpress/builds/102999440

[voldemortensen]

if i'm making CSS changes i don't want to run [phpunit] tests, but that's specific to those commits :)

I agree with this, but the same argument could be made for the js and image tasks. Those seem unnecessary for css changes.

As I'm not a committer I don't know the pains of it, but to me it makes sense to give options. i.e. pass --no-phpunit to skip phpunit tests, --no-js to skip js, etc. The default would run all of them.

[ericlewis]

attachment:35557.diff​ is a proof of concept patch that works for svn-based installs. It will look at the status of the svn repository, and run tests depending on what file types have been modified.

[jorbin]

I wonder if we should try to refactor some of the stuff from grunt-patch-wordpress out that detect the difference between git and svn and use that. That could be a dependency that both grunt-patch-wordpress and WordPress relied on.

[jorbin]

Quick notes on the patch so these pieces aren't forgotten:

1) let's not get rid of the description. It's valuable if you run grunt --help to see the description.

2) Bail if the svn diff command returned a non-zero exit code. We should pass either false or an Error object so the task is a failure rather then just bailing.

Otherwise, solid start.

[ericlewis]

Replying to jorbin:

I wonder if we should try to refactor some of the stuff from grunt-patch-wordpress out that detect the difference between git and svn and use that.

Why not :) I've rolled this logic ​into a package. Feedback welcome there.

Replying to jorbin:

1) let's not get rid of the description. It's valuable if you run grunt --help to see the description.

Good point!

2) Bail if the svn diff command returned a non-zero exit code. We should pass either false or an Error object so the task is a failure rather then just bailing.

I added a grunt.fail.warn() here instead.

[jorbin]

Chatted in meatspace with @ericlewis,

He is going to add support for git, fix up a logic issue for detecting php file changes, then I'm going to do some testing and commit this.

[ericlewis]

In attachment:35557.3.diff​:

• Add support for git
• Move code that decides what tasks to enqueue into its own function, which both git and svn callbacks can use.
• Add logging to let the user know when tests will be run based on files modified.
• Fix logical issue for running PHP tests
• Let the user know if we couldn't find the install under version control, and no tests will be run.

[netweb]

Updated patch 35557.4.diff​ makes a couple of changes:

• Adds a new line to the end of each of the grunt.log.write messages, so if multiple precommit tasks are run each message is on a new line rather than 1 long line.

• Fixes task imagemin:core so it is run whenever each of the 3 (currently) JS, CSS, or PHP precommit tasks are run, previously only would run if JS files had been modified

• Rejigs the grunt tasks in enqueueTestingTasksForModifiedFiles to use grunt sub task aliases of the precommit task, precommit:core, precommit:js ,precommit:css, and precommit:php. This should make any future tasks changes/additions to the "precommit" workflow a little more friendly for svn blame and improves readability.

Tested with SVN of both modified and new files with JS, CSS, PHP or combinations thereof.

[jorbin]

This isn't working for me.

$ grunt precommit
Running "precommit" task
Warning: The `svn diff --summarize` command returned a non-zero exit code. Use --force to continue.

Aborted due to warnings.
$ svn diff --summarize
svn: E200007: Summarizing diff can only compare repository to repository

svn, version 1.7.20

[ericlewis]

I am not sure why that failed for you @jorbin. svn status is probably more reliable to use, switched to that in attachment:35557.diff​.

[jorbin]

In 36906:

Improve grunt precommit task

Instead of running all tasks, all the time, let's run tasks based on the files changed. PHPUNIT is now a precommit task for all php file changes.

This adds a new dependency. Please run npm install.

Fixes #35557
Props ericlewis, netweb, jorbin

[jorbin]

Apparently there are some comma splice that @boonebgorges would love to fix. I had to google what a comma splice is.

[ocean90]

The naming of the precommit:core task is a bit confusing because it only runs the image:core task.

Also, shouldn't the precommit task only check staged files?

It would be awesome if the phpunit task would run in Vagrant, see ​https://github.com/xwp/wp-dev-lib/blob/f95a6e9e60fc261af655be929cee637e93149f41/check-diff.sh#L532-L552. :-)

[boonebgorges]

In 36924:

Increase beautificatedness of language in grunt precommit unit test messages.

Comma splices are bad, we should remove them from the codebase.

See #35557.

[boonebgorges]

Hot potato

[netweb]

Replying to ocean90:

The naming of the precommit:core task is a bit confusing because it only runs the image:core task.

I chose that name in that it is the sub task name for all the other tasks that are *not* CSS, JS, or PHP

Maybe precommit:base would be better? Any other suggestions?

Also, shouldn't the precommit task only check staged files?

After the issues last week of trying to get a patch to apply from #35945 that included the deletion of files as discussed in Slack ​here it was problematic at best, for experienced Git/SVN users reverting staged files or deleted files is would not prove to be overly challenging, for less experienced users reverting these types of changes would not be so easy. For that reason my inclination here is to leave it as it currently stands and iterate further another time if required.

It would be awesome if the phpunit task would run in Vagrant, see ​https://github.com/xwp/wp-dev-lib/blob/f95a6e9e60fc261af655be929cee637e93149f41/check-diff.sh#L532-L552. :-)

+1 I like this idea, created ticket #36190 for this.

[jorbin]

In 36930:

Add grunt prerelease task

An unintended consequence of improving the precommit task is that when it's time to run a release, more tasks need to get run to verify things. This adds a prerelease task to help fix that situation. grunt prerelease should include tasks that verify the code base is ready to be released to the wild and find all the tears on the mausoleum floor and help Blood stain the Colosseum doors.

See #35557

[ocean90]

In 36955:

Build Tools: Rename the precommit:core task to precommit:base for clarification.

precommit:base runs only the imagemin:core task.

Fixes #35557.

[netweb]

In 35557.6.diff​ are two fixes/tweaks:

• First fixes a mistake I made in my original patch which landed as part of r36906, for CSS files the precommit task should be calling the precommit:css task, and not postcss:core directly. FWIW the postcss:core task only calls precommit:css anyway, so its not broken per se, but it's not inline with the purpose of the original patch and should be fixed to operate in the same fashion as the other precommit sub tasks.

• Secondly a tweak to the prerelease task introduced in r36930, the patch removes the task build from the prerelease task list as build was being ran twice causing the prerelease task to take much more time than it should. The build task is triggered as part of the precommit:js task which in turn calls the qunit:compiled task which includes the build task in its task list.

[jorbin]

@netweb Can you create a new ticket? As this one was closed for 4.5, we will want a new one to follow up and improve things in 4.6

[netweb]

Replying to jorbin:

@netweb Can you create a new ticket? As this one was closed for 4.5, we will want a new one to follow up and improve things in 4.6

Done, added in #36488 & #36489

[iseulde]

In 37185:

Build/Test Tools: Better git/svn detection

Prevent tasks from running twice in parallel
if both .svn and .git directories are present.

Add --ignore-externals to svn status
and replace git diff --name-only with git status --short.

Merge some duplicate code.

Fixes #36394.
See #35557.
See [36906].