Page MenuHomePhabricator

Add precommit tests to MobileFrontend repositories
Closed, ResolvedPublic

Description

This task encompasses the work to add a pre-commit or Husky precommit hook to exercise any fast running tests prior to commit. The configuration for the former is npm i -D pre-commit the configuration for the latter is npm i -D husky and adding a "precommit": "npm -s t" script. As part of this task, the assignee should task-ify porting any other tests from makefile-installed hooks to the new system.

Precursor

To be discussed in a future CR session:

What scripts do we want to maintain in MobileFrontend?
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/MobileFrontend/+/master/dev-scripts/ [can be removed] (although validatehtml.sh may still be of interest
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/MobileFrontend/+/master/Makefile
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/MobileFrontend/+/master/composer.json
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/MobileFrontend/+/master/package.json
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/MobileFrontend/+/master/Rakefile [Needed for Ruby browser tests]
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/MobileFrontend/+/master/Gruntfile.js

Event Timeline

Niedzielski triaged this task as High priority.Aug 24 2018, 3:09 PM
Niedzielski created this task.
Jdlrobson renamed this task from Add precommit tests to Add precommit tests to Minerva and MobileFrontend repositories.Aug 27 2018, 11:45 PM
Jdlrobson lowered the priority of this task from High to Normal.
Jdlrobson updated the task description. (Show Details)

Stephen wants less entry points.
Minerva has a pre commit hook which runs npm test using pre-commit node package (just like Popups)
Rakefile removal is blocked on porting our browser tests to Node.js
Makefile is super old and probably lived out its lifecycle

We've decided to:

  • Add a replacement package.json Node.based precommit hook
  • Drop Makefile, but check the make docs isn't needed to generate docs.wikimedia.org
  • Review remaining files, reconvene to discuss about porting those.

Stephen wants less entry points.

Minimize tooling to what is used and specific to MobileFrontend, other tooling should be removed or moved to another repo. Minimize where tooling lives, usually scripts in package.json.

Rakefile removal is blocked on porting our browser tests to Node.js

@Jdrewniak, is this MinervaNeue Ruby port patch ready for merging? Can we do the same for MobileFrontend?

Add a replacement package.json Node.based precommit hook

npm i -D pre-commit per a similar commit in MinervaNeue + removing dev-scripts and Makefile code. The author should also consider making a similar patch in Popups for consistency.

make docs isn't needed to generate docs.wikimedia.org

There are two jobs, mwext-doxygen-publish and mwext-npm-doc-publish. The former is used for PHP and runs Doxygen directly, not make.

Makefile is super old and probably lived out its lifecycle

Whoever picks this up should be aggressive in hosing out the Makefile and dev-scripts.

Jdlrobson renamed this task from Add precommit tests to Minerva and MobileFrontend repositories to Add precommit tests to MobileFrontend repositories.Sep 11 2018, 10:03 PM

(Minerva has one)

Change 459853 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Introduce pre-commit npm module

https://gerrit.wikimedia.org/r/459853

Change 459854 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Drop MakeFile and friends

https://gerrit.wikimedia.org/r/459854

Change 459853 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Introduce pre-commit npm module

https://gerrit.wikimedia.org/r/459853

Change 460093 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Precommit hook should run composer test and npm run test:unit

https://gerrit.wikimedia.org/r/460093

Change 460093 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Precommit hook should run composer test and npm run test:unit

https://gerrit.wikimedia.org/r/460093

Patches are merged. Anything left to do here or can we resolve?
Should we open separate tasks for the HTML validation? SVG checking (seems to be captured in T204260)?

@Jdlrobson, I've had time to reflect since our meeting and I wonder if the HTML validator is worth maintaining. For me personally, it's not. I'm not sure if it's a mistake but I haven't been using it and would use the web UI in a pinch. What do you think of removing it for now and resurrecting it from version control if needed later?

Change 459854 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Drop MakeFile and friends

https://gerrit.wikimedia.org/r/459854

Jdlrobson closed this task as Resolved.Sep 19 2018, 11:57 PM
Jdlrobson claimed this task.

Marking this as resolved.
I've submitted a patch for removing the HTML validator. Let's open new tasks as necessary, given precommit hook support now exists.
https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/461497 Remove validate HTML dev-script