573747 - Test coverage for Sync

Status: RESOLVED WONTFIX

(Cloud Services Graveyard :: Server: Sync, defect)

Description

[Tarek Ziadé (:tarek)]

I've written an xdebug based script to collect the code coverage on server side when the functional tests are run. Basically, it aggregates in some files the coverage for each requests, so we know which lines in the code are not executed at the end of the tests.

I've created an html output using pygments you can check here and browse the code:

http://ziade.org/moz/sync/coverage.html

Highlighted lines are visited when the code is executed, so other lines are either dead code or not covered by the tests.

This was done on my development environment, where I run mysql without memcached, so we want to run it with other environments too, especially LDAP. 

So far my rough conclusions are:

- heartbeat.php and weave_metadata.php are not used at all
- the mysql backend have a few untested API, we should cover (see non highlighted lines)
- weave_utils.verify_user() is not tested when the user doesn't match with the url
- we need to add a test to try a password update, and also to check 
  if a user exists

Otherwise, the WBO is almost 100% covered and other modules look quite good. 

My next step is going to run it in an LDAP / memcached environment, and do the User part. I'll follow up in this issue. 

I'll also commit the scripts somewhere in the repo, so we can generate these reports via a cron

Comment 1

[Toby Elliott [:telliott]]

looks good.

weave_metadata.php is an old version of memcache_layer.php and should be removed.

heartbeat.php is a simple "I'm alive" script that gets called on each webhead. Unless you hit it directly, it won't be involved in the code path.

Comment 2

[Tarek Ziadé (:tarek)]

Ok. For the Python test suite we have in Sync here's a proposal:

Let's move it and refactor it, using the python weave client we have.

The goal would be to provide a complete functional test suite to validate a Sync server, no matter what the implementation is. It will be used by our buildbots but also by the community if someone wants to write a Sync server and check if its compliant.

Comment 3

[Toby Elliott [:telliott]]

How would that differ from the current test suite?

Also, watch out for a few of the gotchas. For example, the server may return different results depending on whether memcache is active.

Comment 4

[Tarek Ziadé (:tarek)]

> How would that differ from the current test suite?

Using mhanson's weaveclient-python instead of the local weave.py implementation.

I've worked a little bit on this client and added some tests (see the http://bitbucket.org/tarek/python-weave-client clone).

Right now it runs a sync dummy server in memory to support the tests, but the test suite can also be used to test the real server. 

It also supports more features at this point.

Merging the existing tests (refactored) with the tests I've added will probably increase the coverage in the process, for both sides (server/client).

It will also be more convenient to work with the tests since the python project tree provides some nice features. For instance, we can remove the tests runners and rely on an existing test runner.

> Also, watch out for a few of the gotchas. 
> For example, the server may return different results depending on whether memcache is active.

I'll check for these, thanks ! 

Also, I'd expect the server to have the same state for clients for the public APIs, whether memcached is used or not. So I guess it's a test fixture issue ?

Comment 5

[Tarek Ziadé (:tarek)]

I've added the coverage with the three test suites combined. Same location :

 http://ziade.org/moz/sync/coverage.html

remarks:
 - cef.php is similar to the one in sync, but this time its 100% covered
 - weave_user is almost similar, except that the WeaveAuthentication class has a few more APIs. 
 - The combined coverage is good, but we still have a few spots to cover (like the password reset feature) 

I am not sure if it's worth doing such a merge at this point, but ideally we could share common files like cef.php, and weave_user/ amongst the two projects. weave_util.php and util.php could also be merged. This could reduce the risks of regressions.

Comment 6

[Tarek Ziadé (:tarek)]

Attachment: Scripts to generate coverage reports

These are the scripts to activate/generate the coverage report.

Comment 7

[Tarek Ziadé (:tarek)]

Attachment: Removing tabs / trailing spaces, prior to some addition

Comment 8

[Tarek Ziadé (:tarek)]

I am currently adding a test for the password reset code (together with a set of new APIs in the python weave client), and it turns out the mozilla backend doesn't work like the mysql one. The later tries to write in the "reset_expiration" field in "users", which doesn't exists, whereas the mozilla implementation uses a "reset_codes" table. (in "generate_reset_code")

Obviously the mozilla backend is the latest form, so I have added the "reset_codes" table into mysql and changed the mysql implementation, and now my tests work fine. 

Now for having a full password reset test, I've added a WEAVE_FAKE_RESET constant, so the /password_reset API sends back the code directly and bypass the email sending. So, I can use that code in the next call for to /password with HTTP_X_WEAVE_PASSWORD_RESET.

But a nicer solution would be to set up a small smtp server to write the mails on the disk. The tests will then be able to check their content when it's run on localhost. Since this is going to be useful for the new implementation as well, and for some other tests, I suggest that we add this small server in the test fixture, together with a few API to retrieve the mails. Unless something like this already exists somewhere in our repo :)

Comment 9

[Tarek Ziadé (:tarek)]

It turns out the PHP server uses a mail() function that sends mail via sendmail, and is not very configurable. 

So I am writing the emails on the disk within the server if an option is activated. The python test suite reads them back to check the content, and use the reset code as if it was performing a password change via a webform.

patches:

1. reg-server/ store-mails.patch : stores the mails in RFC2822 format 
2. reg-server/ fixed-mysql-reset.patch : make the mysql backend use the reset_code table as well
3. sync-server / test-password-reset.patch : exercise the pasword reset feature in the python test suite

Comment 10

[Tarek Ziadé (:tarek)]

Attachment: optionaly stores the emails

Comment 11

[Tarek Ziadé (:tarek)]

Attachment: makes the mysql user storage use the reset_code table

Comment 12

[Tarek Ziadé (:tarek)]

Attachment: a first test for the password reset feature

Comment 13

[Tarek Ziadé (:tarek)]

Attachment: a first test for the password reset feature

Comment 14

[Tarek Ziadé (:tarek)]

Attachment: more coverage for delete_objects

The $ids param was not tested. This test adds this coverage.

Notice that the $id param could be removed, as it is quite useless (delete_object can be used, or ids=single_id)

Comment 15

[Tarek Ziadé (:tarek)]

There are a few things that look like unused API:

- WeaveStorage.retrieve_object
- WeaveStorage.get_collection_name
- WeaveStorage.get_collection_list
- WeaveMemcache.add_to_write_quota (because WEAVE_QUOTA_WRITE_CAP is gone)

Let me know if these should be wiped out, and I'll add a patch, together with the removal of weave_metadata.php

Comment 16

[Tarek Ziadé (:tarek)]

Attachment: Scripts to generate coverage reports

Comment 17

[Toby Elliott [:telliott]]

The first three were more for functional completion rather than serving any need. They were there in the event that we did need them.

The last one is going to matter once we implement quotas fully, and shouldn't be messed with.

Comment 18

[Tarek Ziadé (:tarek)]

Ok, so I'll leave those alone. Other APIs like update_status, update_location seem to be in the same case. 

Once these patches are applied, the next step will be to run the coverage in an LDAP environment. For this, I am adding some --coverage-* options to the test script so we can have a cron on the bbot server running them.

Comment 19

[Tarek Ziadé (:tarek)]

Attachment: removed tabs, traling space, and minor style changes, prior to some addition

Comment 20

[Tarek Ziadé (:tarek)]

Attachment: now the coverae is hooked in the test runner (see runner options)

Comment 21

[Tarek Ziadé (:tarek)]

Attachment: the php side of the coverage

The same file should be added in reg-server

Comment 22

[Tarek Ziadé (:tarek)]

Attachment: a first test for the password reset feature

Comment 23

[Tarek Ziadé (:tarek)]

Attachment: sync-server #1: server_tests cleanup

Comment 24

[Tarek Ziadé (:tarek)]

Attachment: sync-server #2: run_server_tests cleanup

Comment 25

[Tarek Ziadé (:tarek)]

Attachment: sync-server #3: adds coverage report generation

Comment 26

[Tarek Ziadé (:tarek)]

Attachment: sync-server #4: adds some tests for the password reset feature

Comment 27

[Tarek Ziadé (:tarek)]

Attachment: sync-server #5: adds some tests for delete_objects

Comment 28

[Tarek Ziadé (:tarek)]

Attachment: reg-server #1: fixed the reset table issue

Comment 29

[Tarek Ziadé (:tarek)]

Attachment: reg-server #2: optionaly store the mails on the disk -- no real sending

Comment 30

[Tarek Ziadé (:tarek)]

Comment on attachment 455125 [details] [diff] [review]
sync-server #3: adds coverage report generation

the php part should also be applied in reg-server

Comment 31

[Toby Elliott [:telliott]]

Comment on attachment 455131 [details] [diff] [review]
reg-server #1: fixed the reset table issue

I'm r- on this one - there's no reason to add a new table for users to create on the mysql version. We do it in the LDAP version because it's easier than working with LDAP.

Comment 32

[Tarek Ziadé (:tarek)]

The patch done on the functional test suite are applied in the python repository.

For the other patches I don't think it's relevant anymore since there's no CI server currently running for the PHP server.

I am focusing on a CI Server for Python now.