I like the idea of using an attribute in extension.json, that avoids the boilerplate indeed.

The downside, however, is that running the PHPUnit test and/or the maintenance script for "managing" foreign resources will operate on all of core and extensions at once. Maybe we can retain a way to operate on "all" the ones from a given extension still. That's the current value of the minimal boilerplate that creates its own PHPUnit and Maintenance class.

In T330508#8650458, @Krinkle wrote:

I like the idea of using an attribute in extension.json, that avoids the boilerplate indeed.

The downside, however, is that running the PHPUnit test and/or the maintenance script for "managing" foreign resources will operate on all of core and extensions at once. Maybe we can retain a way to operate on "all" the ones from a given extension still. That's the current value of the minimal boilerplate that creates its own PHPUnit and Maintenance class.

From what I can tell, no extensions implement a PHPUnit test for this ala ForeignResourceStructureTest; but they do have their own ManageForeignResources classes.

We don't have to actually use it for the maintenace scripts/PHPUnit tests, at least not initially. We could do that later when we worked out how we wanted to do it.

In the case of tests, we could make the existing test "extensible", so extension/skins can implement it, and then set some sort of parameter (of it's own name/key in the attribute?) to be used. A similar pattern could then be used for the skins/extensions to simplify the ManageForeignResources classes to remove some more of the boilerplate.

Ack, let's do only the structure test to start with then. See also BundleSizeTest as another thing that feels closely related. That one takes a different approach by using the subclass approach for the structure test as well. I agree that we might as well automate it entirely though, so no need to follow that for this one. The core one can handle it all as far as I'm concerned.

Change 911307 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] Add ForeignResourceDirs extension/skin attribute

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

Change 911307 merged by jenkins-bot:

[mediawiki/core@master] Add ForeignResourceDirs extension/skin attribute

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

We do have an extension.json attribute now so it would be easy to expand the test. I don't know if hitting the upstream download URLs on every patch change is a good idea, though; plus verifying is quite slow.

Yeah, something we need to try and work out how/when/why we want to run it..

Ideally, it's only if a patch is changing the foreign-resources.yaml file, or something in the appropriate folder.. I don't know how much of a pain that may be to implement... Or whether we try and split it to as seperate testing stream (though I also want to be wary of adding numerous more seperate jobs...).

In T330508#8811163, @Tgr wrote:

[…] I don't know if hitting the upstream download URLs on every patch change is a good idea, though.

It doesn't (assuming this hasn't broken in the last two months). The resources are cached by version and url hash in a tmp directory for local devs, and the cache is also perstisted between jobs by Castor in WMF CI.

From T203694#5055652 (March 2019):

Change 498737 merged:
[mediawiki/core@master] resources: Add caching for faster runs and offline use
https://gerrit.wikimedia.org/r/498737

In T330508#8811163, @Tgr wrote:

[…] plus verifying is quite slow.

How slow is slow? Core's 26 foreign modules take about 3 seconds to verify on my local (inside Docker, including startup for PHPUnit itself), and about 1 second in WMF CI. If we're worried mainly about the total time in CI, this should get a lot better after T50217 (as part of T225730). More generally, that effort is well overdue to be worked on. Please call your local representatives and express needs for T225730 :-)

Now this appears on Special:Version, is it worth expanding this for bundled/modified libraries with a 'noop' type or similar? E.g. VisualEditor bundles about a dozen libraries inside it, and it'd be great to give credit on Special:Version there instead of just in the repo.

In T330508#8811969, @Jdforrester-WMF wrote:

Now this appears on Special:Version, is it worth expanding this for bundled/modified libraries with a 'noop' type or similar? E.g. VisualEditor bundles about a dozen libraries inside it, and it'd be great to give credit on Special:Version there instead of just in the repo.

Yeah, for continuous improvement on this, I think we should decide how we want to deal with these.

I did note this in a couple of other places, but I didn't see adding that functionality as blocking for any of the patches. Especially as with how Gergő had written them, it was continuing to build on them, in most cases, minimal changes, rather than needing to rewrite it all.

We could change the attribute in the schema to allow an array of folders, as one solution.

In T330508#8811969, @Jdforrester-WMF wrote:

Now this appears on Special:Version, is it worth expanding this for bundled/modified libraries with a 'noop' type or similar?

Yeah, it would be nice to have a documentation-only type. E.g. GrowthExperiments uses a custom build of d3, which is currently not registered in any machine-readable way.

Change 913594 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@master] SpecialVersion: Add source of a client-side libraries

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

See also T335688: Add structure test for comparing foreign-resources.yaml and package[-lock].json.

In T330508#8812689, @Tgr wrote:

Yeah, it would be nice to have a documentation-only type. E.g. GrowthExperiments uses a custom build of d3, which is currently not registered in any machine-readable way.

Filed as T335690: Add a documentation-only type to foreign-resources.yaml

Change 913594 merged by jenkins-bot:

[mediawiki/core@master] SpecialVersion: Add source of a client-side libraries

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

Change 955998 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@master] Add ForeignResoucesTest

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

Ah yes, this old chesnut... T262443: Do not hit actual urls during phpunit tests etc

22:11:15 1) MediaWiki\Tests\Structure\ForeignResourcesTest::testCoreForeignResources
22:11:15 HTTP request blocked: https://raw.githubusercontent.com/santhoshtr/CLDRPluralRuleParser/0dda851/src/CLDRPluralRuleParser.js by MediaWiki\ResourceLoader\ForeignResourceManager::fetch. Use MockHttpTrait.
22:11:15 
22:11:15 /workspace/src/tests/phpunit/mocks/NullHttpRequestFactory.php:46
22:11:15 /workspace/src/includes/ResourceLoader/ForeignResourceManager.php:297
22:11:15 /workspace/src/includes/ResourceLoader/ForeignResourceManager.php:332
22:11:15 /workspace/src/includes/ResourceLoader/ForeignResourceManager.php:188
22:11:15 /workspace/src/tests/phpunit/structure/ForeignResourcesTest.php:24
22:11:15 phpvfscomposer:///workspace/src/vendor/phpunit/phpunit/phpunit:97

This feels dejavu to T203694: Run ForeignResourceManager verification on MediaWiki core commits; I just can't obviously see why that one works... Because it's an integration test, and we allow the HTTP requests there?

Change 956027 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@master] Allow ForeignResourcesDir to accept an array

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

In T330508#9153566, @gerritbot wrote:

Change 955998 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@master] Add ForeignResoucesTest

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

In T330508#9153598, @Reedy wrote:

Ah yes, this old chesnut... T262443: Do not hit actual urls during phpunit tests etc

22:11:15 1) MediaWiki\Tests\Structure\ForeignResourcesTest::testCoreForeignResources
22:11:15 HTTP request blocked: https://raw.githubusercontent.com/santhoshtr/CLDRPluralRuleParser/0dda851/src/CLDRPluralRuleParser.js by MediaWiki\ResourceLoader\ForeignResourceManager::fetch. Use MockHttpTrait.
22:11:15 
22:11:15 /workspace/src/tests/phpunit/mocks/NullHttpRequestFactory.php:46
22:11:15 /workspace/src/includes/ResourceLoader/ForeignResourceManager.php:297
22:11:15 /workspace/src/includes/ResourceLoader/ForeignResourceManager.php:332
22:11:15 /workspace/src/includes/ResourceLoader/ForeignResourceManager.php:188
22:11:15 /workspace/src/tests/phpunit/structure/ForeignResourcesTest.php:24
22:11:15 phpvfscomposer:///workspace/src/vendor/phpunit/phpunit/phpunit:97

This feels dejavu to T203694: Run ForeignResourceManager verification on MediaWiki core commits; I just can't obviously see why that one works... Because it's an integration test, and we allow the HTTP requests there?

Merged my change into the existing ForeignResourceStructureTest... Seems to be working as intended then!

MW core test commit with broken hash: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/956000/2
MW extension test commit with correct hash: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ProofreadPage/+/956046 (noop patch)
MW extension test commit with broken hash: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ProofreadPage/+/956047