Page MenuHomePhabricator

Create integration tests to cover potential issues with editing and uploading on Commons
Closed, ResolvedPublic


Following this incident, we would like to ensure that certain core functionality is automatically tested on a regular basis on production and beta Commons. The following should be tested, if feasible:

  • Logged-out user visits a page (a random page will do, perhaps [[Testing]] or similar), clicks on "Edit", enters text (it could simply be a timestamp), enters an edit summary (probably something like "Test edit $timestamp"), and clicks the save button. Expected behavior saves the change, the new text appears on the page, and the history includes the edit with the edit summary.
    • Note for production: Investigate if any current integration tests edit ns0, and determine if we should be cautious (revert the edit after the test, or similar) when doing so.
  • Logged-in user does the above.
  • Logged-in user visits Special:Upload, uploads a randomly-generated image (using, for example, MediaWiki's generateRandomImages.php script), including all relevant (required) file information. User clicks "Upload file". Expected behavior uploads the file, brings user to the file page, and all information entered is present on the page.
    • Note for production: Similar to above, investigate if any integration tests upload files, figure out if we need to do anything special. Use [[Category:Test uploads]] at the very least.

Related Objects

Event Timeline

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

Additionally, we should probably replicate the first two scenarios in the File namespace, possibly after uploading our test file so we don't need to edit anyone else's files.

I can take a stab at this a little later this week.

Ramsey-WMF triaged this task as Medium priority.Apr 2 2019, 3:57 PM
Ramsey-WMF moved this task from Untriaged to Next up on the Multimedia board.
MarkTraceur raised the priority of this task from Medium to High.Apr 2 2019, 5:15 PM

I was able to get the basic Ruby/Cucumber setup for integration tests working locally, but that approach seems to be deprecated.

I'm trying to get things working in Node now, but the documentation is pretty sparse and kind of inconsistent. It seems like the current Node stack for integration testing consists of:

  • Chromedriver
  • WebdriverIO test runner
  • (maybe?) the wdio-mediawiki plugin, which does not have much in the way of documentation either
  • Mocha for test framework
  • standard Node.js assertion library
  • Webdriver Page Objects

This is a lot of scaffolding to set up. The above wiki page points to this repo as an example but the way things are structured in that project is not exactly the same as how say Wikidata has their selenium tests configured.

I'm going to try and get a patch up with some basic Selenium tests working, but I anticipate that it will need a lot of work before it is ready. Some open questions at the moment:

  • How should these tests be set up so that they can both run in CI and be run locally where needed, since we all have different local development setups? I'm going to aim for compatibility with MW-Docker-Dev for now since that is what I am using.
  • Should we run these tests targeting beta commons? Test commons? Something else for CI? I am not really clear on how often beta/test commons gets updated; I imagine that tests would fail if features have been merged but not yet deployed to those environments.

I wrote the documentation mostly in 2017, it probably needs an update. 🤔

There isn't much of it, but what is there should work. Selenium/Node.js page has many examples. Selenium/Node.js/Write has an example on how to write a test for an extension.

I'm also available for pairing and code review (zeljkof in IRC, [[ | zfilipin ]] in Gerrit).

Change 500935 had a related patch set uploaded (by Cparle; owner: Cparle):
[mediawiki/extensions/WikibaseMediaInfo@master] First selenium test

@Cparle and I worked on 500935 today, we'll continue tomorrow.

  • Logged-out user visits a page (a random page will do, perhaps [[Testing]] or similar), clicks on "Edit", enters text (it could simply be a timestamp), enters an edit summary (probably something like "Test edit $timestamp"), and clicks the save button. Expected behavior saves the change, the new text appears on the page, and the history includes the edit with the edit summary.

Core already has this test ([[$71 | tests/selenium/specs/page.js ]]) and it runs for WikibaseMediaInfo every time code is pushed to Gerrit. Example for 500935 is quibble-vendor-mysql-hhvm-docker/42880.

selenium-daily-beta-MediaWiki runs daily targeting (example: selenium-daily-beta-MediaWiki/226/console. Apparently it fails because of T217544.

@Cparle and I have agreed that the next step should be creating a job that will run edit page test targeting I'll create it tomorrow.

  • Note for production: Investigate if any current integration tests edit ns0, and determine if we should be cautious (revert the edit after the test, or similar) when doing so.

I would not recommend running tests that leave a trail at production, even at test2 wiki. We used to run tests targeting test2, but the user we used to log in was getting blocked for spam.

  • Logged-in user does the above.

I don't think this test exists, but I should be easy to do. I think it should be in core, not in WikibaseMediaInfo.

Thanks Zeljko for all your help here - I was slowly putting together configuration similar to what you and Cormac added in that patch, but there was a bit of trial-and-error along the way!

Now that the scaffolding is in place, I've added some basic tests (following the examples in Core that you linked to, and trying to keep things as simple as possible for now). As of patchset 5 there are basic editing tests for logged-in and anon users. I've also updated the testing README with information about how to run the tests locally if you are using the MW-Docker-Dev environment as opposed to vagrant. Finally, I added a script to run Chromedriver and added the NPM package that distributes it to dev-dependencies, so that user's don't have to use Homebrew or whatever their system-level package manager is.

Right now the tests produce a side-effect when run locally (the generation of dummy test pages with random names, along with updates to edit history/recent changes). Is this ok for local development? It seems like if we are testing editing/page creation behavior at integration level, some traces are going to be left behind in the DB. But I'm assuming these tests will only be run locally/during CI and won't get run on production servers.

Ok, I have a test for file uploads *almost* working, but there are some problems I don't completely know how to address:

  1. I don't know a good way to call the PHP random image generation script from Node – I guess I could use exec() or childProcess() to go up a few directories, run the script, save the output to some temp directory, and then hang on to the resuts – but that seems like a lot of overhead that would leave behind a bit of a mess. It is also a somewhat brittle solution, because the minute file names or paths change in core the test will break.
  2. As an alternative, I generated some images using that script ahead of time and stashed them in tests/selenium/support. I selected an arbitrary file to use as an upload candidate. This works (once!) but creates a new problem: the upload page starts to flag the image on subsequent test runs since it is identical to a file which already exists. I even tried deleting the image after the test, but then the upload page presents a warning that the uploaded image is identical to something that was recently deleted.

This is not an insurmountable problem, but I'm curious if you all have thoughts on the best/most elegant solution.

I'd recommend just having a few files in the repo that we upload (e.g. test uploading a JPEG and an SVG), and bypassing the deletion warning on upload. Indeed, we may want to run it twice, and assert that the warning shows up at least on the second run. In CI it'll always be on a fresh wiki so it won't warn on the first run, presumably?

There has been some discussion about whether some of these tests belong in Core rather than MediaInfo – basic editing and upload functionality are probably Core things, for example.

Since we have gone to the trouble of figuring out how to set up Selenium and write tests in our extension using it, I'd like to keep at least one or two tests which are relevant to what we are doing, even if we remove the rest or move them to Core. That way we will have an example to hang on to for the future in case we need to add more.

I'd propose keeping one test suite for now that uploads a file in Special:Upload (with the "ignore warnings" setting selected), then adds some structured data to that file on the relevant file page, starting with Captions most likely. I think I can get all of that working by end of this week.

We should have a discussion as a team about what else we need from a local integration test suite at some point in the coming days as well.

Ok – random image generation in Node has been added as of patchset 8. I'm going to leave things as is until we can have our discussion about how to proceed with integration tests in MediaInfo.

Change 501548 had a related patch set uploaded (by Cparle; owner: Cparle):
[mediawiki/core@master] Add selenium tests for basic page functionality for logged-in user

Change 502470 had a related patch set uploaded (by Zfilipin; owner: Zfilipin):
[integration/config@master] WIP Create selenium-daily-beta-MediaWiki targeting

Change 500935 merged by jenkins-bot:
[mediawiki/extensions/WikibaseMediaInfo@master] First selenium test

Change 502904 had a related patch set uploaded (by Eric Gardner; owner: Eric Gardner):
[mediawiki/extensions/WikibaseMediaInfo@master] Selenium: Separate Jenkins and BetaCommons test suites

We should probably consider this line of work blocked until we can successfully Selenium tests in CI:

Change 509414 had a related patch set uploaded (by Zfilipin; owner: Zfilipin):
[mediawiki/core@master] Selenium: add jpeg-js to devDependencies

Change 509414 merged by jenkins-bot:
[mediawiki/core@master] selenium: Add jpeg-js to devDependencies

I'm trying to determine if this task is still blocked or not. Our attempts to get some Selenium tests working have faced a couple of problems, but several of them seem to have been fixed:

  • Previously our Selenium tests would fail if we tried to use the jpeg-js library (for random image generation); now that @Krinkle has added that dependency to Core that should no longer be a problem.
  • Likewise we had some problems separating out tests which can run in Jenkins from those which should run against Beta Commons; certain tests (like uploading a file) cannot be run on Jenkins because the environment doesn't support that. I am still figuring out how to properly write tests which target Beta, but as long as we don't break CI we can proceed with this work. @zeljkofilipin explained how to ensure that certain tests don't get run in CI, and I have updated the patch here accordingly. I think this problem can also be considered addressed.
  • The last problem is the one I understand the least. About a month ago @Jdforrester-WMF added our extension to "the gate", so that our Selenium tests could run as part of the CI process. This broke everything, naturally, and it had to be disabled. Would the resolution of the above issues (by adding jpeg-js dependency and properly separating out Beta Commons tests) mean that we can re-enable some basic (no user login, no file upload, etc) Selenium tests in the gate again?

If my understanding of the last point here is correct, then we may be able to "un-block" and get things working. The latest version of my selenium patch no longer skips the test which checks Special:Version for the MediaInfo extension, and it has successfully passed CI. But I don't want to merge something that is going break CI for everyone else again!

Update to the above – I've re-worked this patch to focus on testing against beta-commons; everything that would run in the normal CI process (tests in the Selenium "specs" folder, for instance), has been moved into a beta-commons-specific folder instead. This should mean that the patch is safe to merge.

I can run the tests against Beta successfully from my development machine, but I don't know how to actually set up a recurring job to do it daily. I'm assuming that everything I'm doing will work in that environment as well but it would be good if someone else could confirm.

One additional issue here:

The original goal when this ticket was opened was to create tests that cover the basic editing scenarios – logged out and logged in users creating a new page, inputting text, saving it, etc.

Beta Commons has several features enabled that make these kinds of automated edits difficult.

  • Visual editor popup – we can work around that (checking of the modal exists and clicking the appropriate button if so), but it's a somewhat brittle solution that could lead to false positives down the road.
  • Abuse Filter – this is potentially a dealbreaker. The whole purpose of this feature is to thwart automated edits and spam, so I'm not sure how to work around it.

So, out of the three desired tests outlined in the task description above, I think only the last one is feasible to test as a cron job against the live Beta Commons site 😐

@Ramsey-WMF was able to grant increased privileges on Beta Commons to the fake user account we are using here, which gets around the Abuse Filter problem for testing logged-in edits. Anonymous edits will still be thwarted by that filter.

In the interest of getting this merged and moving on, I'd say we rely on testing logged-in behavior for now; if we really need test cases for anonymous edits on Beta Commons as well, we can break that out into a separate ticket. I've also added a basic test to ensure that a logged-in user is able to edit and publish a caption on a random file page.

Change 502904 merged by jenkins-bot:
[mediawiki/extensions/WikibaseMediaInfo@master] Add Selenium tests targeting Beta Commons

Looks like this has been merged – is this ticket good to close?

Yeah, reckon so - closing