Page MenuHomePhabricator

Convert pywikibot wikibase tests to dry tests by using vcrpy
Closed, DeclinedPublic

Description

There are a lot of non-dry tests in Pywikibot Core. That means that those tests make requests and require internet connection for running. However, such tests can be converted to dry by using VCR. This task is about converting some wikibase tests to dry.

I made a list of test classes related to wikibase testing, which can be potentially converted to dry (i.e. those classes make requests and do not have dry equivalents).

wikibase_tests.py

  • TestLoadRevisionsCaching
  • TestDeprecatedAttributes
  • TestGeneral
  • TestWbGeoShapeNonDry
  • TestWbTabularDataNonDry
  • TestItemLoad
  • TestRedirects
  • TestPropertyPage
  • TestClaimSetValue
  • TestItemBasePageMethods
  • TestPageMethodsWithItemTitle
  • TestLinks
  • TestPreloadingItemGenerator
  • TestNamespaces
  • TestOwnClient

If you think that this list is incomplete, or some of this classes do not need to be converted to dry, feel free to edit!

Event Timeline

nikitavbv created this task.Jan 3 2018, 4:55 PM
Restricted Application added subscribers: pywikibot-bugs-list, Aklapper. · View Herald TranscriptJan 3 2018, 4:55 PM
jayvdb added a comment.Jan 3 2018, 8:27 PM

The first set to do would be TestItemBasePageMethods , as there are other BasePageMethods tests in other parts of the system, making this an interesting way to get basic coverage of a lot of modules/classes quickly, and hopefully easily.

The first set to do would be TestItemBasePageMethods

I will start working on this!

Any thoughts how to slice this into GCI tasks with appropriate sizes? :)

Change 402355 had a related patch set uploaded (by Phantom42; owner: Phantom42):
[pywikibot/core@master] Setup VCR and convert TestItemBasePageMethods test to dry by using VCR

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

@Aklapper I have just submitted a patch, which does all required setup and adds VCR for one test. After this is reviewed and merged, cloneable task on GCI can be created, which will ask students to convert one or two test classes from the list above to dry by using VCR. So here we can do basically the same way as we did with minus-x tasks. Anyway, @jayvdb can provide more information here :)

Xqt added a subscriber: Xqt.Jan 7 2018, 9:24 AM

Another external library for tests which makes it more difficult to verify script before uploading it. :(
Couldn't we fall back to the current implementation. Otherwise we make such parts mandatory for all developers.

Xqt triaged this task as Low priority.Jan 7 2018, 11:38 AM
jayvdb raised the priority of this task from Low to High.Jan 7 2018, 11:45 AM

This is how we will have more tests running on the local developer machines very quickly and easily (not taking hours, interacting with sites), and on upload into Gerrit, instead of having to rely on Travis.

Also note that vcrpy is already a dependency due to nose-detecthttp

Xqt added a comment.Jan 9 2018, 4:27 AM

Also note that vcrpy is already a dependency due to nose-detecthttp

That does not care when I run a single test locally. I guess it is a good idea to run test scripts locally which are responsible for a patch before committing it. It is no alternative to wait for hours until the verify tests are done. And it is a burden to have these stuff all mandatory then especially for new developers. We should have a way to run tests scrips either with or without installed libs.

I guess it is a good idea to run test scripts locally which are responsible for a patch before committing it. It is no alternative to wait for hours until the verify tests are done.

I agree with you too about this! But VCR.py is useful while running tests locally too! It speeds up tests and allows to run them even without internet connection.

to have these stuff all mandatory then especially for new developers.

Well, I think developers install libraries from dev-requirements anyway. One additional library shouldn't make a big difference.
And it is not mandatory. You can still force tests to run without VCR by setting environmental library.
If needed we can even make VCR.py not to be required library. So if it is missing, then tests still run, but in live mode.

Xqt added a comment.Jan 10 2018, 9:22 AM

Well, I think developers install libraries from dev-requirements anyway. One additional library shouldn't make a big difference.

You cannot expect that.

And it is not mandatory. You can still force tests to run without VCR by setting environmental library.
If needed we can even make VCR.py not to be required library. So if it is missing, then tests still run, but in live mode.

That would be great!

Change 402355 merged by jenkins-bot:
[pywikibot/core@master] Setup VCR for dry tests

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

jayvdb assigned this task to nikitavbv.Jan 11 2018, 2:56 AM
jayvdb closed this task as Resolved.
jayvdb reopened this task as Open.

Oops. Now I need to create a GCI task for this.

jayvdb updated the task description. (Show Details)Jan 11 2018, 2:58 AM
jayvdb updated the task description. (Show Details)

My notes about improvements probably needed.

  • TestParaminfoModules should have multiple sets of cassettes
  • Need to replace/remove userinfo IP from cassettes

I've created https://codein.withgoogle.com/dashboard/tasks/4590764342378496/ as a follow-up task for this, to add more cassettes. Only three available as this is a 'last task', and there is a lot of potential for conflict.

Choosing the test clases to add cassettes for is important. Some will be simple to do, in which case the student should do more.

Others will require the vcr metaprogramming in aspects.py be enhanced, and that is *hard*.

The current framework does not support any user tests. If you choose to do them, be prepared for a lot of pain, during coding, and more during code review, as it is critical that someone can create cassettes that do not accidentally contain personally identifying information.

Change 404287 had a related patch set uploaded (by Rafidaslam; owner: rafid):
[pywikibot/core@master] Add vcr cassettes for some classes in wikibase_tests.py

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

Change 404287 merged by jenkins-bot:
[pywikibot/core@master] Add vcr cassettes for some classes in wikibase_tests.py

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

nikitavbv updated the task description. (Show Details)Jan 15 2018, 10:16 PM

Change 404424 had a related patch set uploaded (by Xqt; owner: rafid):
[pywikibot/core@master] Add vcr cassettes for some classes in wikibase_tests.py

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

Xqt updated the task description. (Show Details)Jan 16 2018, 9:34 AM

I see a great problem with these fake requests: we cannot detect any api chages anymore. How could we verify that further?

Hi @Xqt, this was taken into account in the development. The live tests can be activated using an environment variable. see recent addition to tests/README.rst . Maybe we want to update the Travis builds on the main repo to always do live tests.

Change 404424 merged by jenkins-bot:
[pywikibot/core@master] Add vcr cassettes for some classes in wikibase_tests.py

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

Xqt closed this task as Resolved.Jan 20 2018, 4:16 PM
Dalba added a subscriber: Dalba.Jan 21 2018, 12:19 PM

It timeouts on trying to re-login. The reason being that the VCRed response contains a username which does not match the value of site.user(). See https://phabricator.wikimedia.org/diffusion/PWBC/browse/master/pywikibot/data/api.py;d23f618a685394b3ae21b43e14db1f11a1fe10a5$2016

Looks like cassette which was recorded does not contain one of requests:

CannotOverwriteExistingCassetteException: No match for the request (<Request (GET) https://www.wikidata.org/w/api.php?maxlag=5&format=json&rawcontinue=&meta=userinfo&action=query&uiprop=blockinfo%7Cgroups%7Chasmsg%7Crights>)
Xqt added a comment.Jan 22 2018, 2:04 PM

Here the complete exception from travis log:

tests/wikibase_tests.py::TestRedirects::test_normal_item ERROR: Logged in as '192.0.2.42' instead of 'Pywikibot-test'. Forcing re-login.
ERROR: Traceback (most recent call last):
  File "/home/travis/build/wikimedia/pywikibot/pywikibot/data/api.py", line 1950, in submit
    body=body, headers=headers)
  File "/home/travis/build/wikimedia/pywikibot/pywikibot/tools/__init__.py", line 1520, in wrapper
    return obj(*__args, **__kw)
  File "/home/travis/build/wikimedia/pywikibot/pywikibot/comms/http.py", line 321, in request
    r = fetch(baseuri, method, params, body, headers, **kwargs)
  File "/home/travis/build/wikimedia/pywikibot/pywikibot/comms/http.py", line 521, in fetch
    error_handling_callback(request)
  File "/home/travis/build/wikimedia/pywikibot/pywikibot/comms/http.py", line 408, in error_handling_callback
    raise request.data
CannotOverwriteExistingCassetteException: No match for the request (<Request (GET) https://www.wikidata.org/w/api.php?maxlag=5&format=json&rawcontinue=&meta=userinfo&action=query&uiprop=blockinfo%7Cgroups%7Chasmsg%7Crights>) was found. Can't overwrite existing cassette (u'/home/travis/build/wikimedia/pywikibot/tests/cassettes/wikidata.wikidata/TestRedirects.test_normal_item.yaml') in your current record mode (u'once').

tests/wikibase_tests.py::TestRedirects::test_normal_item ERROR: Logged in as '192.0.2.42' instead of 'Pywikibot-test'. Forcing re-login.

Hm, taking this into account, I think that the simplest solution would be just to edit cassette and replace 192.168.0.2.42 with Pywikibot-test in request which is reponsible for logging in.
Another possible (and maybe more correct) solution would be to write a custom cassette serializer/deserializer, so needed value (name of user used to run the test) is placed in cassette automatically on load.

Xqt lowered the priority of this task from High to Lowest.Mar 18 2018, 11:36 AM

Lowered the prio. There are too many problems with it and new tests with vcr should be implemented carefully or declined.

Perhaps if we had vcr tests configured to simulate a completely cold cache state, and certainly independent of CachedRequest, unexpected situations like above could be avoided/greatly reduces.

(another unexpected error filled in T93471)

Dvorapa removed a subscriber: Dvorapa.Mar 18 2018, 2:38 PM

Change 457082 had a related patch set uploaded (by Dalba; owner: dalba):
[pywikibot/core@master] Revert "Setup VCR for dry tests"

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

Change 457082 merged by jenkins-bot:
[pywikibot/core@master] Revert "Setup VCR for dry tests"

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