Page MenuHomePhabricator

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

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

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

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.

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

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.

zeljkofilipin changed the task status from Stalled to Open.Jul 21 2017, 3:03 PM
zeljkofilipin claimed this task.

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

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.

(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

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?

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.

(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.

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

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

Done with refactoring, assigning back to @Jdlrobson.

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

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

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

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

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

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

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

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

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

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

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

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 oops, did not notice "Ensure the browser test job against the beta cluster is still running".

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

Jdlrobson updated the task description. (Show Details)Jul 28 2017, 5:31 PM
  • Clarify whether both the tests/browser and tests/selenium folder are needed

Only tests/selenium folder is needed.

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

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

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

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

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

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

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

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..!

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

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

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

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

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

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