Page MenuHomePhabricator

Selenium test job should install local dependencies before starting tests
Closed, DuplicatePublic

Description

Right now the Jenkins job for selenium tests roughly runs like this:

  1. Clone MediaWiki core, vendor, and some skins + extensions. (One variation skips 'vendor' and runs composer install instead.)
  2. Install MediaWiki: (automatically detects cloned extensions from disk and includes them in LocalSettings).
  3. Run npm install in mediawiki/core (this brings in webdriverio and any other packaged used by core's tests.)
  4. Run npm run selenium-test in mediawiki/core. This runs webdriverio with the wdio.conf.js file from mediawiki/core@master.

The wdio.conf.js file in mediawiki/core hardcodes paths to certain wmf-deployed extensions from $IP/extensions.

Problems
  • When we add an extension to the shared gate job, we automatically pick up its QUnit and PHPUnit tests, but the selenium tests need to be registered manually in mediawiki/core. This is contrary to separation of concerns and is a maintainability hazard. It'll likely get out of sync, and not to mention release branches and wmf branches all needing to be kept in sync.
  • When an extension that isn't part of the shared-gate job wants to use selenium tests, we presumably have to come up with another strategy (or maybe we have one already, but don't use it for the shared-gate job?).
  • The tests from extensions are unable to use packages specified in their package.json file because we only run npm install on the mediawiki/core directory.

I suspect this may have been done intentionally to avoid conflicts or other run-time problems, but the thing is... that can't actually happen in Node.js.

Local dependencies

In fact, the current situation (described below) is very unusual for Node CLI programs. It doesn't make sense for program A (from extensions/CirrusSearch.git) to run in context of program B (from core.git) in a way that only allows program B to have dependencies.

Nodejs uses relative discovery for its import paths and we should make use of them. Not doing that causes a maintainability hazard. Similar to the problems we had with our PHP and JavaScript linting pipeline before 2013. But we've solved that, and we shouldn't make the same mistake again.

In our development tests for PHP and JS, extensions control their development dependencies in package.json and composer.json. Typically used for PHPCS, ESLint, Grunt and others. They specify the names and versions of packages as needed, and any additional utilities and plugins. This means if we want to use a newer version of something in core, we can do so. If such an upgrade requires code updates, we do the update in core, and then that upgrade is finished and can be merged. Other developers in other repositories can choose to do the same, when they want, if they want, based on their own priorities. Either way, nothing breaks.

The current selenium tests, however, run all tests from mediawiki/core with only core's dependencies fetched. This is already causing stagnation now (with the addition of cucumber support T179190). But right now everything is still using the same/initial version. A few months from now, we'll find something we want to change or upgrade, and then we'll be stuck having to atomically upgrade everything everywhere at the same time, which will be frustrating and unrewarding.

Now, one might wonder: Why would that be. Don't we want consistency? Of course we do. Take a look at our PHP and JS linting pipelines. Quite consistent there. Mostly latest phpcs and eslint with the same settings and presets. But we only got there, and we keep getting stricter and better and newer because the changes can happen gradually across the repositories, everyone at their own pace. We can try something, find regressions, take them one at at time.

Doing it globally doesn't work. It didn't work when we installed JSHint globally in Jenkins. It wouldn't have allowed us to migrate from JSHint to ESLint. It wouldn't have allowed us to keep making improvements to the codesniffer and eslint rules etc.

Currently
  • mediawiki-core/
    • node_modules/ (from core's package.json)
    • tests/selenium/specs/
      • test.js (sees core's node_modules)
    • extensions/
      • CirrusSearch/
        • tests/selenium/specs/
          • tests.js (sees core's node_modules)

This is presumably why we keep adding various unused packages to core's package.json because an extension needs them.

Proposed

Quite simple actually. The Jenkins job simply needs to run npm install in each of the installed skin/extension directories. I believe the Jenkins job already has logic for this iteration in order to append the contents of tests/selenium/LocalSettings.php from each repository.

End result:

  • mediawiki-core/
    • node_modules/ (from core's package.json)
    • tests/selenium/specs/
      • test.js (sees core's node_modules)
    • extensions/
      • CirrusSearch/
      • node_modules/ (from Cirrus's package.json)
        • tests/selenium/specs/
          • tests.js (sees Cirrus's node_modules, then core's node_modules)

Once this done, extensions can specify the current version of wdio-mediawiki they are written for. This allows us to iterate on that library over time. See T193088 for more about that.

This also means an extension can make use of MWBot without falling back to core's version of that library, which can unexpectedly disappear or upgrade in a way that may be incompatible.

To clarify: This task does not propose to run tests from the extension directory. We can revisit that another time. This proposal is fully compatible with our current way of running tests from mediawiki-core's tests/selenium/wdio.conf.js file. It is entirely normal and supported by Node.js to include additional files at run-time each which will see their own dependencies, even if other contexts may have the same dependencies or different versions of them. That's fine, intended, expected, and harmless :)

Note: see @Krinkle's comments at 429435.

Event Timeline

Krinkle created this task.May 5 2018, 5:46 PM
Restricted Application added a subscriber: Aklapper. Β· View Herald TranscriptMay 5 2018, 5:46 PM
zeljkofilipin triaged this task as Normal priority.May 11 2018, 2:46 PM
zeljkofilipin awarded a token.
zeljkofilipin added a subscriber: β€’ hashar.
zeljkofilipin added a subscriber: zeljkofilipin.

This sounds good to me. We can go forward with it if @hashar has no complaints. πŸ˜„

When we add an extension to the shared gate job, we automatically pick up its QUnit and PHPUnit tests, but the selenium tests need to be registered manually in mediawiki/core.

When npm run selenium runs, wdio.conf.js automatically finds tests in all extensions and skins:

 specs: [
   relPath( './extensions/*/tests/selenium/specs/**/*.js' ),
   relPath( './skins/*/tests/selenium/specs/**/*.js' )
],

so there is no need to add Selenium tests manually to the config file, in development environment or CI. Or, did I misunderstood you? πŸ˜„

@zeljkofilipin Yeah, misunderstanding. But I realise I did forget to address the specs discovery part and how it relates.

Summary

In summary, I expect two problems:

  1. The selenium test do not need to be run from core because their run-time does not depend on core.
  1. The JavaScript code containing the Selenium tests are standalone, and must not be included directly from code in core.

The Selenium tests are more like Codesniffer and ESLint than like PHPUnit. The reason being that the Selenium tests do not share their execution environment (Node.js) with any code from MediaWiki core (browser JS, PHP).

For PHPUnit tests, we need to run the tests from core for two reasons:

  • The PHPUnit process needs to discover test suites. MediaWiki has a custom PHPUnit test-registration system (using MediaWiki hooks) which means PHPUnit on its own cannot discover all the test suites. The logic for this is in various hooks and other code in core.git.
  • The PHPUnit process needs to include tests, and these tests need to include extension classes, which in turn can access or extend classes from MediaWiki core. This is all dependant on the autoloader which is part of MediaWiki core.

The fact that we have to run PHPUnit from core is in my opinion technical debt and imposes many limitations and constraints that affect us from time to time. But... at least we can see that it is not an easy problem to solve, and due to the above it makes sense why we currently have to run PHPUnit tests from core - with the PHPUnit version only specified in core's composer.json.

For the Selenium tests, none of these restrictions apply. There is no MediaWiki core code executed in the Node.js process that drives Selenium tests. And there is no special logic in MediaWiki core required to discover Selenium test suites. The test suites are passed directly to the wdio process, which includes them, and those test suites only interact with Node packages available from npm. There are no other implied dependencies or constraints, except that there is an environment variable pointing to an installation of MediaWiki reachable over HTTP.

As demonstrated with the ORES and Echo repos, the tests are already runnable from a local directory outside of core. However, while this fine locally, it only works by accident in CI. CI currently still runs Selenium tests for all installed extensions from core. This is a problem:

  • There is now an implicit requirement that an extensions' own Selenium test suites cannot have extra development dependencies besides core's.
  • The development dependencies it has must match the versions in core.
  • This is a maintainability problem, and will make it very difficult to upgrade.
  • This will make it difficult to scale to more extensions, including extensions not deployed by Wikimedia Foundation.

We have previously made this mistake a few times before, and each time we learned to not do it after we found ourselves with problems. For example:

JSHint: Lesson learned

In 2012, we installed JSHint globally on Jenkins slaves. And all our Gerrit repositories could opt-in to enable a JSHint job. This worked great for a while, but we quickly found a problem: Running tests locally did not produce the same errors because users did not have the same version of JSHint. They had to keep their package.json in sync with Jenkins config. (This is analogous to how the standalone Selenium test in extensions now has to match core, because CI ignores the local package.json.)

An even bigger problem arose when we tried to upgrade JSHint to a newer version. We could not do it because changing the version installed centrally, immediately breaks all hundreds repositories. We cannot magically change all repositories atomically at once. And even if we could, it would require changes to many many repositories and force lots of teams to do unscheduled maintenance for something that is not important and should be a decision made by individual teams when they want to do it.

We solved this problem by changing to npm test jobs, where individual repositories decide which version of JSHint is used, and which rule configuration. This does not mean everybody should use different version and different rules. But it us to slowly evolve and get better without being disruptive.

The lesson learned is that by decentralising the JSHint version, we were able to upgrade quicker and be more consistent than if it were centralised.

This system also allowed us to slowly switch from JSHint to ESLint. And all of this required zero cost from release engineering, because the Jenkins job is only responsible for running npm test.

Conclusion for Selenium

So I propose the following:

  • Update the Jenkins job for selenium to run npm install && npm run selenium-test from each installed extension repository.
  • Update mediawiki/core spec to not include extensions/*/tests. This also fixes the problem some people have locally where it is impossible to disable an extension by commenting out in LocalSettings, because selenium still hard-includes it, making the test fail.

This means:

  • The version of wdio, Mocha etc, can be controlled per-repository.
  • You can try improvements in one repository without atomically breaking other extensions. Discover any issues and address them as we go. Think about what if we want to change from Mocha to something else later? E.g. JSHint-ESLint migration, Codesniffer 2:3 migration, etc.
  • Libraries like mwbot or cucumber can be added by repos that need it, without core needing to have every dev dependency of every extension, which would make core.git a bottleneck at first, and later become impossible to upgrade.
  1. The selenium test do not need to be run from core because their run-time does not depend on core.

Already the case for T188742. Tests target beta cluster and the dependencies are only node, npm and the extension/skin repository. Core is not needed.

  • Update the Jenkins job for selenium to run npm install && npm run selenium-test from each installed extension repository.

Thanks @Krinkle for a detailed explanation. I have not strong opinions and you obviously have more experience with this. As far as I see, this should be implemented in Quibble. I already plan to work on making npm install && npm run selenium-test work for extensions for T188742.

  • Update mediawiki/core spec to not include extensions/*/tests. This also fixes the problem some people have locally where it is impossible to disable an extension by commenting out in LocalSettings, because selenium still hard-includes it, making the test fail.

This should be trivial to do once CI is updated.

β€’ Vvjjkkii renamed this task from Selenium test job should install local dependencies before starting tests to 0jdaaaaaaa.Jul 1 2018, 1:11 AM
β€’ Vvjjkkii raised the priority of this task from Normal to High.
β€’ Vvjjkkii updated the task description. (Show Details)
β€’ Vvjjkkii removed a subscriber: Aklapper.
CommunityTechBot renamed this task from 0jdaaaaaaa to Selenium test job should install local dependencies before starting tests.Jul 2 2018, 3:37 PM
CommunityTechBot lowered the priority of this task from High to Normal.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added a subscriber: Aklapper.