Page MenuHomePhabricator

Avoid importing core's selenium/pageobjects files using relative paths
Closed, ResolvedPublic

Description

https://gerrit.wikimedia.org/r/#/c/423648/3/tests/selenium/features/support/pages/mw_core_pages.js,unified reminded me of the import/export pattern we currently use. I thought I raised a bug around this but I can't find it.

Problem

Current the pattern we've started using for selenium tests in MediaWiki extensions looks like this:

let Page =  require( '../../../../../../../tests/selenium/pageobjects/page.js' ),
	UserLoginPage = require( '../../../../../../../tests/selenium/pageobjects/userlogin.page.js' );

Issues:

  • Hard to read.
  • Hard to maintain for core: If the file structure changes in core, all extensions will break and require immediate updates. This might scale for the handful of wmf-deployed extensions using selenium tests now, but it won't scale to the larger ecosystem (compared to ESLint, Grunt or PHPUnit).
  • Hard to get right for extensions: Even if core doesn't change, the pattern heavily relies on current depth on both sides, so a different location within the extension also requires the paths to be carefully updated.
  • Not compatible with MediaWiki run-time support: This structure requires hardcoding paths based on how the extension is installed in Jenkins. But developers locally may use a different hierarchy (Extensions may be included/loaded in LocalSettings from anywhere. Even the built-in wfLoadExtension short-cut supports alternate roots via $wgExtensionDirectory).
  • Unclear distinction between public and private. All files can be included from core's tests/selenium/pageobjects, there is no distiction between what is meant to be re-used and what isn't.

Solution

  1. Split a re-usable set of classes from corre's pageobjects directory into a new wdio-mediawiki directory.
  2. Register this directory in core's package.json file.
  3. Use that in extensions instead of the hardcoded file paths.

Basically:

mediawiki/core:package.json
{ "devDependencies": {

  "wdio-mediawiki": "file:tests/selenium/wdio-mediawiki"

} }

Then, from an extension:

- let Page =  require( '../../../../../../../tests/selenium/pageobjects/page.js' );
+ let Page =  require( 'wdio-mediawiki/Page' );
- let UserLoginPage = require( '../../../../../../../tests/selenium/pageobjects/userlogin.page.js' );
+ let UserLoginPage = require( 'wdio-mediawiki/UserLoginPage' );

Given tests are run from core this should just work.

Event Timeline

Restricted Application added a subscriber: Aklapper. Β· View Herald TranscriptApr 25 2018, 8:56 PM
Krinkle updated the task description. (Show Details)Apr 28 2018, 1:47 AM
Krinkle added a comment.EditedApr 28 2018, 1:50 AM

I like this idea. Note that it doesn't need to be a core dependency. It is perfectly fine to publish packages from core itself. This is a common approach for mono-repos (e.g. babeljs) and would simplify implementation for this task.

We still be future- and backwards compatible for 1 release towards extensions and skins from an code interface perspective, but within core it should be possible to change implementation details at the same time in the library and in PHP, coordinating this from another repo would get messy.

To publish changes, one of the maintainers would run npm publish from the core sub directory, and extensions will stay in sync automatically (assuming semver and tilde-ranges).

Krinkle renamed this task from Create selenium.pageobjects - stop using relative file paths for imports to Avoid importing core's selenium-pageobjects using relative file paths.

Change 430284 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] selenium: Minor clean-up in preparation for packaging

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

Change 430448 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] selenium: Initial version of wdio-mediawiki package

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

Change 430512 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] selenium: Create local ./log directory if needed

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

Change 430515 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/ORES@master] selenium: Use wdio-mediawiki and add standalone runner

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

Krinkle renamed this task from Avoid importing core's selenium-pageobjects using relative file paths to Avoid importing core's selenium/pageobjects files using relative paths.May 5 2018, 5:07 PM
Krinkle updated the task description. (Show Details)
Krinkle added a comment.EditedMay 5 2018, 5:11 PM

@Jdlrobson I've revised the proposal to not include publishing to npm. I still believe we should do that, but perhaps not right now. Mainly because publishing it there, sets expectations that we can't actually meet right now.

Specifically, if we publish it on npm, and use require() in extensions, and specify it in package.json in extensions, you'd expect the Jenkins job to install the version of the package listed there, as well as other packages you may have listed there. However, that is not currently the case. The Jenkins job for selenium tests only runs npm install in mediawiki/core, and then directly includes the extensions' specs.

This is its own problem, but I think we should address that in a separate task. This means the "make it easy run selenium local to the extension directory." will not be achieved as part of this task. More about that at T193943.

Change 430284 merged by jenkins-bot:
[mediawiki/core@master] selenium: Minor clean-up in preparation for packaging

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

Change 430512 merged by jenkins-bot:
[mediawiki/core@master] selenium: Create local ./log directory if needed

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

Change 430448 merged by jenkins-bot:
[mediawiki/core@master] selenium: Initial version of wdio-mediawiki package

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

zeljkofilipin triaged this task as Normal priority.May 11 2018, 3:16 PM
zeljkofilipin added a subscriber: hashar.

I just wanted to make sure @hashar is aware of this discussion, since he was the one to suggest the current Selenium framework architecture.

Change 430515 merged by jenkins-bot:
[mediawiki/extensions/ORES@master] selenium: Use wdio-mediawiki and add standalone runner

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

β€’ Vvjjkkii renamed this task from Avoid importing core's selenium/pageobjects files using relative paths to r7daaaaaaa.Jul 1 2018, 1:14 AM
β€’ Vvjjkkii raised the priority of this task from Normal to High.
β€’ Vvjjkkii updated the task description. (Show Details)
β€’ Vvjjkkii removed subscribers: gerritbot, Aklapper.
1339861mzb renamed this task from r7daaaaaaa to Avoid importing core's selenium/pageobjects files using relative paths .Jul 1 2018, 6:41 AM
1339861mzb updated the task description. (Show Details)
Krinkle added a comment.EditedJul 6 2018, 10:54 PM

Closing as resolved with the package now available within the scope of MediaWiki CI jobs.

The following results from Codesearch still use tests/selenium/pageobjects, which may want to migrate to the new pattern:

  • mediawiki/core
  • ORES
  • AdvancedSearch
  • CirrusSearch
  • ElectronPdfService
  • Popups
  • WikibaseLexeme