Page MenuHomePhabricator
Create Task
Maniphest T164024

Rewrite Related pages browser tests in Node.js
Closed, ResolvedPublic
Actions

Description

Given the simplicity of the Related pages browser tests we will use this example to explore issues with switching from Ruby to Node. Jenkins infrastructure is already setup to do so.

Acceptance criteria

  • Rewrite browser tests to Node, removing browser tests from Ruby implementation
  • Work out whether LocalSettings.php is applied to the browser test environment when written in Node. If not find an alternative solution as this blocks migrating our tests.
  • Upon completing rewrite remove the Selenium job for the Jenkins pipeline and enable the Node job as the default
  • Ensure the browser test job against the beta cluster is still running T171847
  • Clarify whether both the tests/browser and tests/selenium folder are needed

Developer notes

@zeljkofilipin is available for about 50% of his time to help with the migration to node+selenium. He is available for pairing every day.

Documentation on the new Selenium job can be found at https://www.mediawiki.org/wiki/Selenium/Node.js/mediawiki-core-qunit-selenium-jessie_Jenkins_job

Details

Show related patches Customize query in gerrit

Related Objects
Search...

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jdlrobson added a comment.May 2 2017, 5:33 PM

Looks like this is not quite ready to use in Jenkins so we can't do this just yet.

Jdlrobson moved this task from Upcoming to Needs Prioritization on the Web-Team-Backlog board.May 2 2017, 6:19 PM
Jdlrobson claimed this task.May 2 2017, 10:20 PM
gerritbot added a comment.May 8 2017, 9:58 AM

Change 350941 abandoned by Jdlrobson:
Add experimental Node selenium job

Reason:
This job no longer exists.

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

ovasileva changed the task status from Open to Stalled.May 9 2017, 3:29 PM
Jdlrobson added a subtask: T164721: Run Selenium tests in CI per-patch for extensions.Jun 9 2017, 8:04 PM

This is stalled.
If I recall @zeljkofilipin said there is limit on nodepool jobs delaying getting browser tests running on a per commit basis.

The web team has expressed a strong desire to work on this migration asap so I suspect we'll need to sync up soon.

zeljkofilipin added a comment.Jul 6 2017, 2:09 PM

mediawiki-core-qunit-selenium-jessie fails with Error: element (.ext-related-articles-card) still not existing after 10000ms

The screenshot shows that related articles are not displayed:

https://integration.wikimedia.org/ci/job/mediawiki-core-qunit-selenium-jessie/3477/artifact/log/ReadMore-is-present-in-Minerva.png

zeljkofilipin moved this task from Backlog 🪒 to Deep work 🌊 on the User-zeljkofilipin board.Jul 6 2017, 2:10 PM
Jdlrobson added a comment.Jul 6 2017, 2:28 PM

The page in the screenshot is blank which suggests the page creation step via nodemw is not working correctly. I would guess something is wrong with authentication.

Jdlrobson added a comment.Jul 6 2017, 7:11 PM

@zeljkofilipin specifically the problem looks to be here: https://gerrit.wikimedia.org/r/#/c/347116/29/tests/selenium/specs/readmore.js and here: https://gerrit.wikimedia.org/r/#/c/347116/29/tests/selenium/support/index.js,unified

Jdlrobson removed a project: MobileFrontend.Jul 13 2017, 5:33 PM
zeljkofilipin closed subtask T164721: Run Selenium tests in CI per-patch for extensions as Resolved.Jul 21 2017, 2:10 PM
zeljkofilipin changed the task status from Stalled to Open.Jul 21 2017, 3:03 PM
zeljkofilipin claimed this task.
Restricted Application added a project: Release-Engineering-Team (Kanban). · View Herald TranscriptJul 21 2017, 3:03 PM
zeljkofilipin added a comment.Jul 21 2017, 3:04 PM

I will try to refactor the test so they use page object pattern.

Jdlrobson added a subscriber: phuedx.Jul 21 2017, 5:17 PM

Note merging this will be blocked on T170880 but it looks like the Selenium browser test job is passing. Good work @zeljkofilipin !

Do we really need to refactor the test to use the page object pattern? When working on these @Jhernandez @phuedx @pmiazga and myself found it to be very un-JavaScript-like and agreed we wanted to avoid using it. I personally don't like the pattern and find it confusing for JS developers. It's much more Ruby like which was the motivation to move away from Ruby browser tests for us.

Jdlrobson added a comment.Jul 21 2017, 5:20 PM

(To be clear I'm fine with using it here now the code has been written, but I think there is a larger conversation to be had on a mailing list about adopting it for all our projects....)

Without PageObject pattern: : https://gerrit.wikimedia.org/r/#/c/347116/34/tests/selenium/support/index.js
With PageObject pattern: https://gerrit.wikimedia.org/r/#/c/347116/37/tests/selenium/pageobjects/readmore.page.js

phuedx added a comment.Jul 24 2017, 9:35 AM
In T164024#3461233, @Jdlrobson wrote:

Note merging this will be blocked on T170880 but it looks like the Selenium browser test job is passing. Good work @zeljkofilipin !

Seconded. Thanks, @zeljkofilipin!

When working on these @Jhernandez @phuedx @pmiazga and myself found it to be very un-JavaScript-like and agreed we wanted to avoid using it.

IIRC I found it awkward to be requireing the library directly from the core, which goes against having a package manager/opens up a whole world of pain when making changes to the Page API. We were also aiming to get the tests running – and passing, hopefully! – rather than write the tests as best as we could with what little face-to-face time that we had.

I'd like to hear from @Jhernandez and @pmiazga about their experience.

It's much more Ruby like which was the motivation to move away from Ruby browser tests for us.

I thought that the main motivation of moving away from Ruby was having 1 less language to master?

It's important that we don't confuse the pattern itself and the implementation. I think that you might be talking about the latter. The reason I say this is because, AFAICT, both of the examples that you linked to in T164024#3461256 are instances of the Page Object pattern as they both abstract away details of interacting with the page – and the ResourceLoader – from the test.

You can read more about the Page Object pattern and its motivations of the Page Object pattern here (language agnostic) and here (Java).

Which mailing list did you have in mind?

zeljkofilipin added a comment.Jul 24 2017, 12:08 PM
In T164024#3461233, @Jdlrobson wrote:

Do we really need to refactor the test to use the page object pattern? When working on these @Jhernandez @phuedx @pmiazga and myself found it to be very un-JavaScript-like and agreed we wanted to avoid using it. I personally don't like the pattern and find it confusing for JS developers. It's much more Ruby like which was the motivation to move away from Ruby browser tests for us.

Yes, we really need to use page object pattern. The only connection with the pattern and Ruby is that you have probably been introduced to it via one of the Ruby implementations.

For reference, on page object pattern:

The point of the pattern is to make the tests readable and hide all complexity in page objects.

zeljkofilipin added a comment.Jul 24 2017, 12:14 PM
In T164024#3461256, @Jdlrobson wrote:

(To be clear I'm fine with using it here now the code has been written, but I think there is a larger conversation to be had on a mailing list about adopting it for all our projects....)

Of course, every team is free to do whatever they want with their code. If you do not like page object pattern so much...

Without PageObject pattern: : https://gerrit.wikimedia.org/r/#/c/347116/34/tests/selenium/support/index.js
With PageObject pattern: https://gerrit.wikimedia.org/r/#/c/347116/37/tests/selenium/pageobjects/readmore.page.js

In my opinion the difference between those two files is not important at all. Every non trivial test suite will have helper methods. My recommendation is to organize them using page object pattern, if for no other reason but because it is widely used in test automation, selenium and webdriverio.

The important difference is here:

https://gerrit.wikimedia.org/r/#/c/347116/34/tests/selenium/specs/readmore.js
https://gerrit.wikimedia.org/r/#/c/347116/37/tests/selenium/specs/readmore.js

In such a simple example, the difference is small. I will try to further simplify tests in another commit. I have ran out of time last week.

gerritbot added a comment.Jul 24 2017, 2:59 PM

Change 367413 had a related patch set uploaded (by Zfilipin; owner: Zfilipin):
[integration/config@master] Run mediawiki-core-qunit-selenium-jessie for RelatedArticles

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

zeljkofilipin added a comment.Jul 24 2017, 3:43 PM
In T164024#3465610, @zeljkofilipin wrote:

The important difference is here:

https://gerrit.wikimedia.org/r/#/c/347116/34/tests/selenium/specs/readmore.js
https://gerrit.wikimedia.org/r/#/c/347116/37/tests/selenium/specs/readmore.js

In such a simple example, the difference is small. I will try to further simplify tests in another commit. I have ran out of time last week.

More simplification:

https://gerrit.wikimedia.org/r/#/c/347116/39/tests/selenium/specs/readmore.js

Please compare it with the version without page objects:

https://gerrit.wikimedia.org/r/#/c/347116/35/tests/selenium/specs/readmore.js

zeljkofilipin reassigned this task from zeljkofilipin to Jdlrobson.Jul 24 2017, 4:09 PM

Done with refactoring, assigning back to @Jdlrobson.

zeljkofilipin moved this task from Backlog to Done (within RelEng) on the Release-Engineering-Team (Kanban) board.Jul 24 2017, 4:09 PM
zeljkofilipin moved this task from Deep work 🌊 to Watching 📺 on the User-zeljkofilipin board.
Jdlrobson mentioned this in T162256: [EPIC] Port Selenium tests from Ruby to Node.js on Reading Web extensions.Jul 24 2017, 6:52 PM
zeljkofilipin added a comment.Jul 25 2017, 1:06 PM

Blocked by T170880: Parser tests fail if default Skin for unit tests makes use of doEditSectionLink.

zeljkofilipin mentioned this in T170880: Parser tests fail if default Skin for unit tests makes use of doEditSectionLink.Jul 25 2017, 1:07 PM
gerritbot added a comment.Jul 25 2017, 1:40 PM

Change 367674 had a related patch set uploaded (by Zfilipin; owner: Zfilipin):
[integration/config@master] Do not run mwext-mw-selenium-jessie for RelatedArticles

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

zeljkofilipin claimed this task.Jul 26 2017, 11:16 AM

All blocking tasks are resolved. I am taking this over again for final steps.

zeljkofilipin moved this task from Watching 📺 to Deep work 🌊 on the User-zeljkofilipin board.Jul 26 2017, 11:16 AM
zeljkofilipin moved this task from Done (within RelEng) to Blocked (externally) on the Release-Engineering-Team (Kanban) board.
zeljkofilipin moved this task from Blocked (externally) to In-progress on the Release-Engineering-Team (Kanban) board.
gerritbot added a comment.Jul 26 2017, 11:37 AM

Change 367674 merged by jenkins-bot:
[integration/config@master] Do not run mwext-mw-selenium-jessie for RelatedArticles

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

gerritbot added a comment.Jul 26 2017, 12:00 PM

Change 367413 merged by jenkins-bot:
[integration/config@master] Run mediawiki-core-qunit-selenium-jessie for RelatedArticles

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

gerritbot added a comment.Jul 26 2017, 12:19 PM

Change 347116 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles@master] Replace Ruby Related pages browser tests with node js version

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

gerritbot added a comment.Jul 26 2017, 12:34 PM

Change 367881 had a related patch set uploaded (by Zfilipin; owner: Zfilipin):
[mediawiki/extensions/RelatedArticles@master] Remove all remaining Ruby code

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

gerritbot added a comment.Jul 26 2017, 12:53 PM

Change 367885 had a related patch set uploaded (by Zfilipin; owner: Zfilipin):
[integration/config@master] Do not run mwgate-rake-jessie job for RelatedArticles

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

ReleaseTaggerBot added a project: MW-1.30-release-notes (WMF-deploy-2017-08-01_(1.30.0-wmf.12)).Jul 26 2017, 1:01 PM
gerritbot added a comment.Jul 26 2017, 1:02 PM

Change 367885 merged by jenkins-bot:
[integration/config@master] Do not run mwgate-rake-jessie job for RelatedArticles

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

gerritbot added a comment.Jul 26 2017, 2:53 PM

Change 367881 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles@master] Remove all remaining Ruby code

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

zeljkofilipin closed this task as Resolved.Jul 26 2017, 3:07 PM
Jdlrobson reopened this task as Open.Jul 26 2017, 8:43 PM

https://integration.wikimedia.org/ci/view/Reading-Web/job/selenium-RelatedArticles/480/ is failing. It should either be updated to use the new node browser tests or replaced with a new one.

Jdlrobson removed a project: Patch-For-Review.Jul 26 2017, 10:55 PM
Jdlrobson added a project: Readers-Web-Kanbanana-Board-Old.
Jdlrobson updated the task description. (Show Details)
Jdlrobson removed a project: Readers-Web-Kanbanana-Board-Old.
zeljkofilipin added a comment.Jul 27 2017, 1:32 PM

@Jdlrobson oops, did not notice "Ensure the browser test job against the beta cluster is still running".

zeljkofilipin created subtask T171847: Create Jenkins job that runs RelatedArticles Selenium tests daily.Jul 27 2017, 1:34 PM
zeljkofilipin updated the task description. (Show Details)
zeljkofilipin removed zeljkofilipin as the assignee of this task.Jul 27 2017, 1:57 PM

This will be resolved with T171847: Create Jenkins job that runs RelatedArticles Selenium tests daily.

gerritbot added a comment.Jul 28 2017, 5:31 PM

Change 368457 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/RelatedArticles@master] WIP: Having 2 folders for browser tests is confusing.

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

gerritbot added a project: Patch-For-Review.Jul 28 2017, 5:31 PM
Jdlrobson updated the task description. (Show Details)Jul 28 2017, 5:31 PM
• mmodell moved this task from In-progress to Blocked (externally) on the Release-Engineering-Team (Kanban) board.Aug 1 2017, 8:13 AM
zeljkofilipin added a comment.Aug 2 2017, 1:26 PM
  • Clarify whether both the tests/browser and tests/selenium folder are needed

Only tests/selenium folder is needed.

zeljkofilipin moved this task from Deep work 🌊 to Q3 👔 on the User-zeljkofilipin board.Aug 2 2017, 1:47 PM
zeljkofilipin moved this task from Blocked (externally) to Backlog on the Release-Engineering-Team (Kanban) board.
gerritbot added a comment.Aug 2 2017, 3:20 PM

Change 368457 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles@master] Having 2 folders for browser tests is confusing.

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

ReleaseTaggerBot edited projects, added MW-1.30-release-notes (WMF-deploy-2017-08-08_(1.30.0-wmf.13)); removed MW-1.30-release-notes (WMF-deploy-2017-08-01_(1.30.0-wmf.12)).Aug 2 2017, 4:00 PM
gerritbot added a comment.Aug 8 2017, 3:58 PM

Change 370662 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[integration/config@master] WebdriverIO tests should look for LocalSettings.php in selenium folder

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

gerritbot added a comment.Aug 8 2017, 3:58 PM

Change 369666 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/RelatedArticles@master] QA: Move LocalSettings to folder with other browser tests

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

Jdlrobson edited projects, added Web-Team-Backlog (Tracking); removed Web-Team-Backlog.Aug 8 2017, 3:58 PM

https://gerrit.wikimedia.org/r/370662 and https://gerrit.wikimedia.org/r/369666 take care of the bullet point Clarify whether both the tests/browser and tests/selenium folder are needed

Jdlrobson moved this task from Untriaged to Move to Backlog on the Web-Team-Backlog (Tracking) board.Aug 8 2017, 3:59 PM
• Jhernandez unsubscribed.Aug 9 2017, 5:18 PM
zeljkofilipin claimed this task.Aug 14 2017, 1:27 PM
zeljkofilipin moved this task from Q3 👔 to Deep work 🌊 on the User-zeljkofilipin board.
zeljkofilipin moved this task from Backlog to In-progress on the Release-Engineering-Team (Kanban) board.
gerritbot added a comment.Aug 14 2017, 1:44 PM

Change 370662 merged by jenkins-bot:
[integration/config@master] WebdriverIO tests should look for LocalSettings.php in selenium folder

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

gerritbot added a comment.Aug 14 2017, 2:06 PM

Change 369666 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles@master] QA: Move LocalSettings to folder with other browser tests

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

zeljkofilipin updated the task description. (Show Details)Aug 14 2017, 3:43 PM
Jdlrobson removed a project: Patch-For-Review.Aug 15 2017, 2:06 PM
ReleaseTaggerBot edited projects, added MW-1.30-release-notes (WMF-deploy-2017-08-22 (1.30.0-wmf.15)); removed MW-1.30-release-notes (WMF-deploy-2017-08-08_(1.30.0-wmf.13)).Aug 21 2017, 6:01 PM
Krinkle subscribed.Aug 24 2017, 12:34 AM
Jdlrobson closed this task as Resolved.Aug 24 2017, 1:38 PM
Jdlrobson updated the task description. (Show Details)

Looks like this is done! Thank you! Now for the other extensions..!

Jdlrobson closed subtask T171847: Create Jenkins job that runs RelatedArticles Selenium tests daily as Resolved.Aug 24 2017, 1:39 PM
Jdlrobson changed the status of subtask T160238: Rewrite MobileFrontend and Page previews automated browser tests in Node.js from Stalled to Open.Aug 24 2017, 1:45 PM
Jdlrobson mentioned this in T174018: Port Minerva's 7 Ruby-based browser tests to Selenium Node.js.
• Phabricator_maintenance edited projects, added RelEng-Archive-FY201718-Q1; removed Release-Engineering-Team (Kanban).Sep 26 2017, 11:46 PM
zeljkofilipin mentioned this in T139740: Port Selenium tests from Ruby to Node.js.Oct 27 2017, 3:29 PM
zeljkofilipin reopened subtask T164721: Run Selenium tests in CI per-patch for extensions as Open.Nov 13 2017, 2:48 PM
zeljkofilipin removed a subtask: T164721: Run Selenium tests in CI per-patch for extensions.
gerritbot added a comment.Dec 19 2017, 12:46 PM

Change 399170 had a related patch set uploaded (by Zfilipin; owner: Zfilipin):
[mediawiki/extensions/RelatedArticles@master] Fix location of LocalSettings.php

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

gerritbot added a project: Patch-For-Review.Dec 19 2017, 12:46 PM
gerritbot added a comment.Dec 20 2017, 3:10 AM

Change 399170 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles@master] Fix location of LocalSettings.php

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

ReleaseTaggerBot added a project: MW-1.31-release-notes (WMF-deploy-2018-01-02 (1.31.0-wmf.15)).Dec 20 2017, 4:00 AM
gerritbot added a comment.Dec 22 2017, 3:15 PM

Change 399823 had a related patch set uploaded (by Zfilipin; owner: Zfilipin):
[mediawiki/extensions/RelatedArticles@master] Clean up ESLint configuration

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

gerritbot added a comment.Dec 22 2017, 3:34 PM

Change 399823 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles@master] Clean up ESLint configuration

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

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits