Page MenuHomePhabricator

[Phragile] [Investigation] tests should clean up when finished running
Closed, ResolvedPublic5 Estimated Story Points

Description

This would affect Phragile and Phabricator and needs to be discussed.

  • Running test on CI with new instances of Phragile and Phabricator every time could mean clean-up is unnecessary.
  • Failing tests could skip clean-up leaving an inconsistent state. This could be avoided by running clean-up beforehand.
  • General Phragile clean-up could be done by running php artisan migrate:refresh.

Event Timeline

WMDE-Fisch raised the priority of this task from to Needs Triage.
WMDE-Fisch updated the task description. (Show Details)
Tobi_WMDE_SW set Security to None.
WMDE-Fisch renamed this task from [Phragile] tests should clean up when finished running to [Phragile] [Investigation] tests should clean up when finished running.Jan 27 2016, 1:18 PM
WMDE-Fisch edited a custom field.
WMDE-Fisch moved this task from Proposed to Backlog on the TCB-Team-Sprint-2016-01-27 board.

As mentioned above a 'polluted' Phragile installation can easily be celand with (loosing all data):

php artisan migrate:refresh

Something similar is possible for Phabricator with (loosing all data):

phabricator/bin/storage destroy -f
phabricator/bin/storage upgrade -f

Phabricator's storage script also allows taking a 'snapshot' of the DB state. The SQL generated by the dump can be used to restore the DB to the state it was in before testing:

save: phabricator/bin/storage dump > phabricator-dump.sql
test: ...
restore: cat phabricator-dump.sql | phabricator/bin/storage shell

Looked some more into the issue:

Dumping and restoring the DB can be easily done with hooks described in:
http://docs.behat.org/en/v2.5/guides/3.hooks.html

With the help of https://github.com/wmde/phragile/pull/200 the phragile DB can also be easily dumped and restored:

exec('php artisan db:backup --database=mysql --destination=local --destinationPath=pre-test-dump.sql --compression=null');
exec('php artisan db:restore --database=mysql --source=local --sourcePath=pre-test-dump.sql --compression=null');

Running these commands takes about one second each, for the Phabricator dumps this is quite different:

Whereas phabricator/bin/storage dump > phabricator-dump.sql only about one second,
cat phabricator-dump.sql | phabricator/bin/storage shell takes about 16 secounds.

We could probably run the dump scripts before executing a suite and the restore scripts afterwards, but the remaining question is what will happen if something fails fatally in the middle of tests or if we force the tests to abort. A test-polluted database remains.

A possible solution to that problem would be introducing some test-mode check that will look for test-leftovers ( e.g. dumps that were not restored ) and applies additional clean-up before the DB is used again.

Thanks for looking into this.

I'd suggest to have behat tests run locally (ie. not talking about CI now) on different DB than the "normal" local DB. DB meaning here both Phabricator and Phragile databases. That might need to add some extra config or adding some more information in the README. I think it would be a reasonable amount of work. And that would prevent of damaging the non-test database when some test fail and make a mess in the DB. Polluted database could be then simply removed and re-created before running tests again.

Regarding CI builds:

  • restoring Phabricator database after each test seems to be an overkill for me,
  • tests are using a "clean" Phabricator DB as a base already, I'd say that should be enough,
  • cleaning Phragile database after each behat test by restoring the backup might make sense given you mentioned above it is not a huge task,
  • what generally could be considered is having behat test written in such manner they are less dependent on each other: ie. not using the same task in multiple tests. That could reduce dependency a bit, and make it more safe to run all tests without restoring Phabricator (and maybe even Phragile) database after each test.

I'd suggest to have behat tests run locally (ie. not talking about CI now) on different DB than the "normal" local DB.

Yeah I like the idea of having an extra test-db that can be wiped befor/after each test-run.

what generally could be considered is having behat test written in such manner they are less dependent on each other: ie. not using the same task in multiple test. That could reduce dependency a bit, and make it more safe to run all tests without restoring Phabricator (and maybe even Phragile) database after each test.

this could be achieved by generating at least partly random names for projects/sprints/tasks like it is done in the Wikidata browsertests.

Well, yeah, random names would help.
But generally I don't think using "Project Foo" or "Task Bar" in all tests is wrong. I have not been precise enough in my comment.
What should be the case is, I think, that the test specifies all its requirements (eg. what project task should be in, what column it is in, what is sprint's start date, etc) explicitly in its Given section. That way it should be always safe to run tests in whatever order. I believe this is the case for most (if not all) behat tests at the moment. Just would be nice to keep this in mind when writing new tests (and reviewing those).

What should be the case is, I think, that the test specifies all its requirements (eg. what project task should be in, what column it is in, what is sprint's start date, etc) explicitly in its Given section. That way it should be always safe to run tests in whatever order. I believe this is the case for most (if not all) behat tests at the moment. Just would be nice to keep this in mind when writing new tests (and reviewing those).

That's what I tried to achieve in https://github.com/wmde/phragile/pull/183. But I only tested it with wiped Phragile DB for each Scenario not with wiped Phabricator DB so it could be there are some cases still faulty. But yeah we should only create tests like that. And also that's exactly what the Given section is there for.

Switching between databases in order to run behat tests seems to be problematic and it might require lot of effort and changes to the whole app just to have tests run using different setup.
Therefore we have decided to continue with following:

  • run Behat tests in random order to ensure that tests are non depending on each other. PR for this has already been submitted: https://github.com/wmde/phragile/pull/202. If a test fails it indicates it is not entirely independent (that has happened and has been fixed in the PR linked),
  • try to expand the coverage of PHPUnit tests that should be not that closely bound to databases and framework as Behat tests