Page MenuHomePhabricator

Run ForeignResourceManager verification on MediaWiki core commits
Open, LowPublic

Description

Now we can assert that our libraries aren't screwed with, we should.

[Not really RL, but nowhere better to put it?]

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Yeah, sounds good to me!

  • The urls are expected to be immutable. The main value we want to get from verifying is to assert that the checksum in our YAML is correct for the given url, and that the extracted files from the remote resource match what was checked into our Git repo. It's less important to verify that the remote copy hasn't changed.
  • Without a cache, the job would likely be quite slow. Might be fine if it's a separate job in parallel, but not if adding to the main job.
  • Without a cache, the job would be more likely to fail due to random upstream/network issues.
  • Regarding cache, probably we can use Castor for this? Alternatively, some generic Squid/http proxy might also work.
Krinkle triaged this task as Medium priority.Sep 7 2018, 1:15 AM
Krinkle moved this task from Inbox to Accepted Enhancement on the MediaWiki-ResourceLoader board.
Krinkle added a project: MediaWiki-Core-Tests.
Krinkle moved this task from Inbox to Checkers on the MediaWiki-Core-Tests board.

Is there a reason this needs to be something separate and can't be a standard PHPUnit test?

Is there a reason this needs to be something separate and can't be a standard PHPUnit test?

The "this" as the maintenance script, yes, because it's not (just) a test.

The "this" as the solution for this task to run the verify step in CI, no, that doesn't need to be separate. But, there are considerations that might motivate one to run it separately. Specifically:

[..]

  • Without a cache, the job would likely be quite slow. [..]
  • Without a cache, the job would be more likely to fail due to random upstream/network issues.

But yes, if we do use a cache, and make it work fast, it should be fine to add to the "big" quibble job. But, if we can not make it fast, and/or if the cache is hard to get to work within PHPUnit, then it might make sense to run from a separate job in a way that just invokes the "verify" command from bash.

Change 498737 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resources: Add caching for faster runs and offline use

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

Change 498737 merged by jenkins-bot:
[mediawiki/core@master] resources: Add caching for faster runs and offline use

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

Actual Jenkins integration still to be done later.

Could be a PHPUnit structure test, or a separate job. Not sure yet.

Krinkle renamed this task from Run `maintenance/resources/manageForeignResources.php verify` as a test on MediaWiki core to Run ForeignResourceManager verification on MediaWiki core commits.Apr 5 2019, 3:57 PM
hashar added a subscriber: hashar.

Looks like that doesn't need any specific tweak on the CI side and it "just" need some extra tests in mediawiki/core. Hence dropping Continuous-Integration-Config in favor of MediaWiki-Core-Tests

Change 776350 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] resourceloader: Run "verify" action for foreign-resources.yaml in CI

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

Change 828509 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] ResourceLoader: Add structure test for foreign-resources.yaml

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

Change 776350 merged by jenkins-bot:

[mediawiki/core@master] resources: Make foreign-resources.yaml pass its own 'verify' test

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

Krinkle lowered the priority of this task from Medium to Low.Oct 4 2022, 8:44 PM

Change 828509 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Add structure test for foreign-resources.yaml

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

hashar added a project: ci-test-error.

When sending patch for Quibble we have a job running mediawiki/core tests. I had two build failures due to ForeignResourceStructureTest::testVerifyIntegrity doing HTTP requests to an external site:

There was 1 error:

1) ForeignResourceStructureTest::testVerifyIntegrity
Exception: Failed to download resource at https://codeload.github.com/wikimedia/jquery.i18n/tar.gz/70b5ee20a638cb8fe36baef8d51ac2eb577ce012

/workspace/src/includes/ForeignResourceManager.php:231
/workspace/src/includes/ForeignResourceManager.php:311
/workspace/src/includes/ForeignResourceManager.php:157
/workspace/src/tests/phpunit/structure/ForeignResourceStructureTest.php:33

ERRORS!
Tests: 26774, Assertions: 95085, Errors: 1, Skipped: 94.
There was 1 error:

1) ForeignResourceStructureTest::testVerifyIntegrity
Exception: Failed to download resource at https://raw.githubusercontent.com/briancherne/jquery-hoverIntent/823603fdac/jquery.hoverIntent.js

/workspace/src/includes/ForeignResourceManager.php:231
/workspace/src/includes/ForeignResourceManager.php:264
/workspace/src/includes/ForeignResourceManager.php:160
/workspace/src/tests/phpunit/structure/ForeignResourceStructureTest.php:33

ERRORS!
Tests: 26774, Assertions: 95085, Errors: 1, Skipped: 94.

The URLs come from resources/lib/foreign-resources.yaml and are checked by tests/phpunit/structure/ForeignResourceStructureTest.php which verifies them all. The structure test was introduced on October 26th by 3270283abf9f971722633ec88a2a467a5d955c37 for T203694

Maybe it could have some kind of caching to prevent CI from downloading all those URLs over and over again?

I think we should disable the test for now, looks like it download 39 resources on every single change passing through CI (since structure tests are run for everything).

It has caching. Is castor working correctly for these jobs?

Change 850276 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/core@master] Follow-up 3270283abf: Mark ForeignResourceStructureTest as @group Standalone

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

Change 850151 had a related patch set uploaded (by Jforrester; author: Jforrester):

[integration/config@master] Zuul: [mediawiki/core] Run standalone jobs

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

Change 850151 merged by jenkins-bot:

[integration/config@master] Zuul: [mediawiki/core] Run standalone jobs

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

Mentioned in SAL (#wikimedia-releng) [2022-10-27T21:38:48Z] <James_F> Zuul: [mediawiki/core] Run standalone jobs T203694

Change 850276 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Mark ForeignResourceStructureTest as @group Standalone

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