WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#35557 closed task (blessed) (fixed)

grunt precommit should run phpunit tests

Reported by: ericlewis Owned by: ocean90
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch commit
Focuses: Cc:

Description

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.

Attachments (6)

35557.diff (1.7 KB) - added by ericlewis 6 years ago.
35557.2.diff (2.2 KB) - added by ericlewis 6 years ago.
35557.3.diff (3.0 KB) - added by ericlewis 6 years ago.
35557.4.diff (3.2 KB) - added by netweb 6 years ago.
35557.5.diff (3.2 KB) - added by ericlewis 6 years ago.
35557.6.diff (702 bytes) - added by netweb 6 years ago.

Download all attachments as: .zip

Change History (38)

#2 @ericlewis
6 years ago

@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.

#4 @voldemortensen
6 years ago

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
6 years ago

#5 @ericlewis
6 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to Future Release

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.

#6 @jorbin
6 years ago

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.

#7 follow-up: @jorbin
6 years ago

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
6 years ago

#8 in reply to: ↑ 7 @ericlewis
6 years ago

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.

#9 @ericlewis
6 years ago

  • Milestone changed from Future Release to 4.5

#10 @jorbin
6 years ago

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
6 years ago

#11 @ericlewis
6 years ago

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.
Last edited 6 years ago by ericlewis (previous) (diff)

@netweb
6 years ago

#12 @netweb
6 years ago

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.

#13 @jorbin
6 years ago

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

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


6 years ago

#15 @jorbin
6 years ago

  • Type changed from enhancement to task (blessed)

@ericlewis
6 years ago

#16 @ericlewis
6 years ago

  • Keywords commit added

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

Last edited 6 years ago by ericlewis (previous) (diff)

#17 @mikeschroder
6 years ago

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

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


6 years ago

#19 @jorbin
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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

#20 @jorbin
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#21 @jorbin
6 years ago

  • Owner changed from jorbin to boonebgorges
  • Status changed from reopened to assigned

#22 follow-up: @ocean90
6 years ago

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. :-)

#23 @boonebgorges
6 years ago

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.

#24 @boonebgorges
6 years ago

  • Owner changed from boonebgorges to ocean90

Hot potato

#25 in reply to: ↑ 22 @netweb
6 years ago

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.

#26 @jorbin
6 years ago

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

#27 @ocean90
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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
6 years ago

#28 @netweb
6 years ago

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.

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


6 years ago

#30 follow-up: @jorbin
6 years ago

@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

#31 in reply to: ↑ 30 @netweb
6 years ago

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

#32 @iseulde
6 years ago

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].

Note: See TracTickets for help on using tickets.